Repository navigation
Conversation
…ld not fill Two seams the error-reporting work of #21, #22 and #26 created the conditions to close and did not. `Failure::message()` led with `id.or(path)`, but a download sets both and `imogen_sdk::Error::Io` wraps a bare `std::io::Error` that carries no path of its own. A full disk therefore said `warning: asset-7: io: No space left on device` — which photograph, and never which file or directory to go and look at. The record keeps both names, so the fix is which one the sentence leads with: a failure that happened here is about the file, one the server sent is about the asset. The marker is not serialized, because the record's shape is the API's own and both names are in it either way. `upload` still rendered a refused album fill with `{error}` onto stderr — the last place in a batch path that dropped the `details` map. Under `--json` it was absent from the document, `addedToAlbums` was silently short, and the run exited 0: a run whose every fill was refused looked successful. Filing what was uploaded is part of the run, so a refused fill is now a `Failure` like every other, named by the album's id where a file's is named by its path, and it participates in the exit rule #27 settled rather than inventing a second one. That leaves a batch holding two kinds of item, and an album is not a file, so a run that sent one file no longer closes with "1 of 2 files failed". The noun widens to the README's own word for a batch's members, and only for a run that had one of the other kind. Both tests were written first and fail on main: the download names the asset, and the upload exits 0. Closes #34 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
First review round. The total the run closes with counted only the files that got through, while the failures counted both kinds: a manifest filing into two albums where one fill is refused did four things and three of them worked, and the sentence called it "1 of 3 items failed", dropping the fill that succeeded. The noun and the total have to move together, because the noun names what is being counted — `download` has summed the files it wrote with the ones it skipped under one noun since #27, and this is the same rule with a second kind of item. Two doc comments in `errors.rs` were left describing a world with one caller. `with_id`'s said "an upload only has the file", which the album fill it now names by id alone contradicts outright, and `for_id`'s named `assets edit` as though it were the only way in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
Code reviewRound 1 (
imogen-cli/src/commands/upload.rs Lines 247 to 250 in 5065516
Lines 80 to 87 in 5065516 Both carry a test that fails without the fix: 🤖 Generated with Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #34.
Two seams left by the error-reporting work of #21, #22 and #26.
1. A download IO failure named the asset and never the file
Failure::message()picked its subject asid.or(path).download.rssets both, andimogen_sdk::Error::Iowraps a barestd::io::Errorthat carries no path of its own, so a fulldisk said:
Shape chosen:
message()prefers the path for a failure that happened here — notdownloaddropping one of the two names. The record is what a script reads, and which asset and which
destination are both part of the answer; dropping either would lose information the run had.
Only the sentence has to choose one to lead with, and the honest rule is where the failure
happened: the server refused this asset, the filesystem refused this file. The marker is
#[serde(skip)], so the--jsonrecord is byte-for-byte what it was.2. An album fill the server refused was a sentence on stderr
upload.rsrendered it asctx.out.warn(format!("Could not fill an album: {error}"))— the lastplace in a batch path formatting with
{error}and discardingdetails. Under--jsonit wasabsent from the document,
addedToAlbumswas silently short, and the exit status was 0.It is now a
Failurelike every other, named by the album's id where a file's failure is named byits path — which is also what tells the two apart in one
failureslist. It participates in therule #27 settled rather than inventing a second one: the file got through, so the run is partial
(exit 3), and
--jsonstill writes exactly one document.That leaves a batch holding two kinds of item. Counting a refused fill among the files closed a
run that sent one file with
1 of 2 files failed, so the noun widens to item — the README's ownword for a batch's members — and only for a run that actually had one of the other kind.
Both audiences, against the release binary
A run with no album is unchanged:
filestays the noun, exit 0 stays exit 0.Tests
Written first, and both fail on
main:a_download_that_could_not_be_written_names_the_file— unit (the record keeps both names) andin
tests/batch_exit_status.rsagainst the real binary. Onmain:warning: asset-1: io: Not a directory (os error 20).an_album_that_could_not_be_filled_is_a_failure_like_any_other— the real binary, bothaudiences. On
main: exit 0 where the rule wants 3.The integration tests go in
tests/batch_exit_status.rs, #27's harness, rather than a second stubserver inside
upload.rs:$?and the bytes on stdout only exist once there is a process.Scope
Nothing on the wire changed and the SDK pin stays at
v0.5.0. #40 (what a--jsonbatch emitswhen there was nothing to do, and the
--dry-rundocument shapes) is deliberately not taken —it is a decision about empty batches and separable from this one. #25 (the TUI surface) is not
taken either.
Found, not fixed
Nothing.
Verify green locally:
cargo test --locked && cargo fmt --check && cargo clippy --all-targets --locked -- -D warnings && cargo build --release --locked.🤖 Generated with Claude Code