fix: surface actionable API errors instead of raw JSON dumps (BUG-2046)#30
Merged
Conversation
HTTPError.Error() previously dumped the raw JSON response body on one line, burying the actionable details/suggestion fields of the standard Wherobots error envelope. Parse the envelope and render it multi-line (code, message, details, suggestion, request id), falling back to the exact prior strings for empty, non-JSON, or non-envelope bodies. Also add APIErrorDetails for callers that need to inspect the details field, and JSONError for machine-oriented callers that want the raw envelope body verbatim.
Dynamic api commands are machine-oriented and already write success responses as raw JSON to stdout. Match that on failure: wrap HTTPError in executor.JSONError so the API error envelope is printed verbatim (parseable JSON on stderr) instead of the human-readable rendering. Unwrap() preserves the *HTTPError for errors.As callers.
BUG-2046: job-runs create failed during script upload with a generic
'request failed with HTTP 400: {raw json}' that hid the actionable
detail (No storage source found for bucket: ...) and gave no hint
about which step failed or what target was attempted.
Wrap upload-url request errors with the attempted s3://bucket/key, and
when the API reports a missing storage source for an explicit
--upload-path / WHEROBOTS_UPLOAD_PATH override, append guidance to
omit the override or use a registered storage integration. Wrapping
uses %w so isRetryable's *HTTPError handling is unaffected.
Review follow-up: a foreign payload like {"errors":["boom"]} previously
matched the envelope detection and rendered with no detail, losing the
raw body. Require at least one errors[] entry with a code or message,
and avoid re-parsing the gjson array twice.
There was a problem hiding this comment.
Reviewed by Salty Hambot 🤖🧂
Solid error-handling plumbing with good test coverage. One real callout: the wrapUploadURLError guidance hinges on a fragile English substring match against server prose — worth a comment acknowledging that tradeoff.
2 finding(s) posted · 2 filtered as false positives.
💬 To request a re-review, comment @salty-hambot review
sfishel18
previously approved these changes
Jun 4, 2026
…lures Address review feedback on the actionable-error surfacing for local script uploads: - Thread the upload-path override source (flag vs WHEROBOTS_UPLOAD_PATH env var) down to wrapUploadURLError so the "missing storage source" guidance names the right knob: "omit --upload-path" for the flag, "unset WHEROBOTS_UPLOAD_PATH" for the env var. Previously the env-var case gave incorrect "omit --upload-path" advice. - Document the fragile free-text match for the "no storage source found" detail with a NOTE explaining the tradeoff (additive guidance, never errors on a miss). - Wrap the presigned-PUT upload failure with the same "uploading script to s3://bucket/key" context the upload-URL path got. - Add TestJobsRunUploadURLStorageSourceErrorEnvOverrideIsActionable mirroring the flag test but driving the override via the env var and asserting the env-appropriate wording (no misleading "omit --upload-path").
sfishel18
approved these changes
Jun 12, 2026
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.
Summary
Fixes BUG-2046:
job-runs createfailed during script upload with a genericrequest failed with HTTP 400: {raw json blob}, burying the only actionable part (InvalidInputException (No storage source found for bucket: ...)) and giving no hint about which step failed or what upload target was attempted.The CLI builds the upload key correctly (verified against the live OpenAPI spec); the 400 is a server-side storage-source decision. This PR fixes the CLI-side problem: error surfacing.
Before
After (curated
job-runscommands)After (dynamic
api ...commands)Machine-oriented
apicommands already emit success responses as raw JSON; they now emit the error envelope as raw, parseable JSON too:Commits
feat(executor): render API error envelopes as readable errors— parse the standard envelope inHTTPError.Error(), multi-line rendering, exact fallback to prior strings for empty/non-JSON/non-envelope bodies; addsAPIErrorDetailsandJSONErrorfeat(api): emit raw JSON error envelope from dynamic api commands— wrapHTTPErrorinJSONErrorin the generated-command pathfix(jobs): explain upload-url failures with target and guidance—prepareScriptwraps upload-url errors with the attempteds3://bucket/key; storage-source guidance only when an explicit--upload-path/WHEROBOTS_UPLOAD_PATHoverride hit a "No storage source found" errorfix(executor): require standard shape before treating body as envelope— review follow-up: foreign{"errors":["..."]}payloads fall back to the raw bodyCompatibility
*executor.HTTPErrorstruct unchanged; wrapping happens afterexecWithRetry, soisRetryable's direct type assertion is unaffected (covered by tests)Testing
JSONErrorunwrap semantics, api-command raw-JSON error, and 3 httptestjob-runs createscenarios (explicit override w/ guidance, non-envelope w/o guidance, managed default w/o guidance)go test ./...,go vet,gofmt,pre-commit run --all-filesall cleanDeferred
Up-front client-side validation of
--upload-pathagainst org storage sources was considered and deliberately skipped: the server is the source of truth, pre-validation risks false blocks, and it would add API calls to a path that intentionally skips them. A non-blocking warning could be a follow-up if desired.