cli: surface the server's error body on a failed upload (not just the status code)#112
Merged
Merged
Conversation
Declared error responses (progenitor's ErrorResponse variant) were
matched on status code only; the catch-all arm printed just "(HTTP
{code})" and dropped the ApiErrorResponse body. A 400 — e.g. the
upload validation that names duplicate step IDs — surfaced as a bare
"failed (HTTP 400)" with no detail. Read .into_inner().error and
include it, for both authenticated and anon uploads. The actionable
401/413/429 hints are preserved.
|
🔍 Preview deployed: https://0e0fab79.toolpath.pages.dev |
akesling
added a commit
that referenced
this pull request
Jun 29, 2026
…ache (#113) `main` went red after #112 with a flaky failure in `cmd_resume::tests::resolve_input_url_uses_cache_on_hit_without_refetching` ("download ... failed (500): boom") — the cache-hit test reached the network despite a seeded cache. Root cause: two independent mutexes guarded the *same* global resource, the process environment. `cmd_pathbase`'s `EnvGuard` serialized via a private `ENV_LOCK`, while `cmd_resume`/`cmd_cache`/`cmd_export`/`config` use `config::TEST_ENV_LOCK`. Both touch `TOOLPATH_CONFIG_DIR`, so under parallel `cargo test` an `EnvGuard::set("TOOLPATH_CONFIG_DIR", dirB)` could run while the cache-hit test held `TEST_ENV_LOCK` with `dirA`, making `config_dir()` read `dirB` → cache miss → download → 500. (`std::env::set_var`/`var_os` are also not thread-safe, so concurrent access across the two locks is a data race on the environ regardless.) The `EnvGuard` SAFETY comment's claim that "no other tests mutate or read TOOLPATH_CONFIG_DIR" was simply false. #112 only changed production upload code; it surfaced a latent flake by chance. Fix: `EnvGuard` now acquires the shared `config::TEST_ENV_LOCK` and the private `ENV_LOCK` is deleted, leaving exactly one env lock in the crate so all env-touching tests are mutually exclusive. (The PATH/HOME resume guards already run only inside `TEST_ENV_LOCK`-holding tests.) Test-only change. Verified: 14 full-suite runs (incl. 6 at 32 threads) green; clippy clean.
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
1 participant
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.
Symptom
A failed upload printed only the status code —
Error: upload to dev/pathstash failed (HTTP 400)— with none of the server's explanatory message. The server does return a useful body (e.g. pathbase's new validation: "path X has N duplicate step ID(s); step IDs must be unique within a path; e.g. …"), but the CLI threw it away.Root cause
The progenitor-generated client splits error responses into two variants:
UnexpectedResponse— status codes not declared in the OpenAPI spec. The CLI already read the body here and surfaced the message.ErrorResponse— status codes the spec declares (for the upload endpoints,400/401/413/429, all typed asApiErrorResponse { code, error }). The CLI matched on the status code and the catch-all armbail!ed with just(HTTP {code}), never touching the typed body.So precisely the codes the API documents — including the
400that names the actual problem — lost their message.Fix
In the
ErrorResponsearm of bothgraphs_post(authenticated) andanon_graphs_post, readresp.into_inner().errorand include it:… failed (HTTP {code}): {message}. The actionable special cases are preserved (the401login hint; the anon413size-cap /429rate-limit hints); everything else now carries the server's human-readable message.Pairing
Complements pathbase#233, which adds the clear 400 for documents with duplicate step IDs, and toolpath#111, which stops generating such documents in the first place. With all three, a malformed upload now fails fast and tells you why.
Verification
cargo build/clippy/fmtclean onpath-cli(the one pre-existingredundant_closurewarning is unrelated and present onmain).Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.