feat(storage): surface cloud-provider error code in upload failures - #492
Conversation
A failed cloud-storage upload previously raised only "Cloud storage upload failed (HTTP 403)" and discarded the provider's response body, hiding the actual denial reason. Diagnosing a real platform-side IAM misconfiguration (GCS AccessDenied naming the service account and the missing storage.objects.create permission) required a temporary code edit to see it. - Log the response body at DEBUG level before raising, truncated to CLOUD_UPLOAD_ERROR_BODY_LIMIT (bodies may embed signed URLs, so they never reach the user-facing message). - Parse a short whitelisted provider error code (GCS/S3 XML <Code>, Azure x-ms-error-code header) and append it to the KeboolaApiError message: "Cloud storage upload failed (HTTP 403, AccessDenied)". - Unit tests: XML/header/no-code extraction, charset whitelist against injected content, 403 message + DEBUG body, success path unaffected.
| # Strict charset: provider error codes are short alphanumeric tokens (GCS/S3 | ||
| # "AccessDenied", Azure "AuthorizationFailure"). Anything else in the response | ||
| # -- signed URLs, credentials, free-form messages -- must never reach the | ||
| # user-facing error string. | ||
| _CLOUD_ERROR_CODE_RE = re.compile(r"<Code>([A-Za-z0-9._-]{1,64})</Code>") | ||
|
|
||
|
|
||
| def _extract_cloud_error_code(response: httpx.Response) -> str | None: | ||
| """Best-effort short error code from a failed cloud-storage response. | ||
|
|
||
| Azure surfaces it in the ``x-ms-error-code`` header; GCS and S3 return an | ||
| XML body with a ``<Code>`` element. Returns ``None`` when neither matches. | ||
| """ | ||
| header_code = response.headers.get("x-ms-error-code", "") | ||
| if header_code and re.fullmatch(r"[A-Za-z0-9._-]{1,64}", header_code): | ||
| return header_code | ||
| match = _CLOUD_ERROR_CODE_RE.search(response.text[:CLOUD_UPLOAD_ERROR_BODY_LIMIT]) | ||
| return match.group(1) if match else None |
There was a problem hiding this comment.
🟡 New functionality added to a source file that is already far over its maximum size limit
New code is added to a file that is already about 3,769 lines long (src/keboola_agent_cli/client.py:3361-3378), well beyond the 2,000-line hard ceiling the project sets for this file, which its own contribution rules say requires splitting the file before merging more functionality into it.
Impact: The change grows a file that the repository's stated policy says must be split before accepting more functionality, so it violates the mandatory contribution guideline.
Rule reference and mechanism
CONTRIBUTING.md "File-size budgets -- split when concerns drift" sets a hard ceiling of 2000 LOC for client.py/manage_client.py and states: "When a file crosses the hard ceiling, splitting is required before merging more functionality into it." The base file was 3736 lines and this PR adds a new module-level helper _extract_cloud_error_code plus a new regex constant and error-path logic, i.e. new functionality, at src/keboola_agent_cli/client.py:3361-3378. Because the file is already ~1.88x over the hard ceiling, the guideline requires an endpoint-family split (e.g. client/storage.py) before adding this.
Was this helpful? React with 👍 or 👎 to provide feedback.
padak
left a comment
There was a problem hiding this comment.
Review of #492 — feat(storage): surface cloud-provider error code in upload failures
Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check, not duplicated here.
Summary
This PR improves diagnostics for cloud-storage upload failures (_upload_to_cloud in client.py, shared by storage file-upload, storage upload-table, and data-app artifact paths): the user-facing KeboolaApiError message now appends the provider's short error code (e.g. Cloud storage upload failed (HTTP 403, AccessDenied)), while the full response body is logged at DEBUG only (--verbose), capped at a new CLOUD_UPLOAD_ERROR_BODY_LIMIT (1500 chars). The extracted code is validated against a strict [A-Za-z0-9._-]{1,64} whitelist before it ever reaches the raised message, specifically to stop signed URLs / credentials embedded in a provider error body from leaking into the user-facing string. Verdict: APPROVE — the change is small, entirely confined to Layer 3 (client.py), does not touch the CLI command surface (no plugin-sync-map drift), is well tested (7 new tests, all verified passing), and make check is fully green.
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 1
- Nits: 1
Blocking findings
(none)
Non-blocking findings
[NB-1] src/keboola_agent_cli/client.py:46 — new helper added to a file already past the hard file-size ceiling
client.py is 3769 LOC, well past the 2000-LOC hard ceiling in CONTRIBUTING.md's file-size budget table ("When a file crosses the hard ceiling, splitting is required before merging more functionality into it"). This PR adds _extract_cloud_error_code (and its regex constant) directly into client.py rather than extracting cloud-upload helpers into a sibling module (the guide explicitly suggests splitting client.py "by endpoint family"). Unlike data_app_service.py/commands/data_app.py (issue #331) and services/sync_service.py (issue #361), there is no open tracking issue for splitting client.py. Given the addition here is small (34 net lines) and self-contained, this isn't a merge blocker, but it's worth filing a tracking issue (mirroring #331/#361) so the debt doesn't keep growing silently with every small PR like this one.
Nits
[NIT-1]src/keboola_agent_cli/client.py:1938— the raised message wraps the f-string in redundant parentheses (message=(f"Cloud storage upload failed (HTTP {response.status_code}{code_suffix})"),);ruff formatdoesn't flag it, but dropping the outer parens would match the surrounding style.
Verification log
gh auth status→ authenticated (an earlier attempt hit a transient tool-classifier hiccup unrelated togh; a follow-upgh pr viewsucceeded, confirming auth).gh pr view 492 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ OPEN,feat(storage):prefix matches the change type (new diagnostic capability), 3 files, +152/-1 ✓.git rev-parse --abbrev-ref HEAD→feat/upload-failure-diagnostics, matches<branch>; working tree clean ✓.gh pr diff 492→ 205 lines; confined tosrc/keboola_agent_cli/client.py,src/keboola_agent_cli/constants.py,tests/test_client.py. Nocommands/,services/,permissions.py, or plugin/doc files touched — no new/removed/renamed CLI command, so the Plugin synchronization map (§3.4) andOPERATION_REGISTRYdo not apply here ✓.- Layer-violation greps (
typer/click/formatter./console.printin the diff,httpx/requestsincommands/) → both empty; allhttpxreferences are inclient.py(Layer 3, correct) andtests/test_client.py✓. grep -cfor magic numbers, rawerror_code="..."strings, bareexcept:,print(, unmasked tokens, newtuple[...]returns → all empty/clean. The new constantCLOUD_UPLOAD_ERROR_BODY_LIMITis properly defined inconstants.py, not inlined ✓. Test fixtures use the901--prefixed fake-token convention (901-55555-fakeTestTokenDoNotUseXXXXXXXX,901-gcs-fakeUploadTokenXXXX) ✓.grep -rn "Cloud storage upload failed"acrosstests/ src/ plugins/ docs/→ only the new test file andclient.pyitself reference the exact string; no other consumer parses it, so the message change is backward compatible ✓.make check→ exit 0.ruff checkclean,ruff format --checkclean (328 files formatted),ty check1 pre-existing warning-only diagnostic (unresolved-importforhatchling...inscripts/hatch_build.py, unrelated to this diff and explicitly non-blocking per CONTRIBUTING.md),SKILL.mdup-to-date, version in sync,check_command_sync.py→ "all 238 CLI commands registered/documented" (unchanged count — confirms no command surface drift), changelog check OK (no version bump, correctly not required for this PR),check_error_codes.pyOK, full suite 4321 passed, 8 skipped ✓ (matches PR description's claim).uv run pytest tests/test_client.py::TestExtractCloudErrorCode tests/test_client.py::TestUploadToCloudErrorDiagnostics -v→ all 7 new tests pass individually, confirming: GCS/S3 XML<Code>extraction, Azurex-ms-error-codeheader extraction, unparseable body →None, charset-whitelist rejection of an injected signed URL, the 403 message carries the code while the full body stays DEBUG-only, the no-code fallback keeps the exact pre-PR message text, and the success path is unaffected ✓.- Read
src/keboola_agent_cli/commands/storage.py:822-841(storage_upload_table) → both--jsonand human modes route through the sameformatter.error(message=exc.message, ...)call, so the enhanced message reaches both output surfaces without a separate human-mode code path to forget ✓. grep -n "verbose" src/keboola_agent_cli/cli.py→ confirmed--verbosesetslogging.DEBUG(cli.py:292), verifying the PR description's claim that--verbosereveals the full provider body ✓.gh issue list --search "client.py split"/"file-size budget"→ confirmed #331 and #361 exist as tracking issues for other oversized files; no equivalent open issue forclient.py, supporting NB-1.- Could not reproduce a live 403 against a real cloud provider (no credentials, and the failure is IAM-misconfiguration-specific per the PR's own repro story) — relied on the unit tests above plus direct code reading, which fully cover the claimed behavior.
Open questions for the author
- The PR description notes "No version bump (code-only, rides with the next release)." Per CONTRIBUTING.md's "Releasing a new version" checklist item 6,
gotchas.mdshould be reviewed for new behavior at the release that ships this — is there a plan to add a one-line entry then (e.g. "upload failures now include the provider's short error code"), or is this considered too minor to need one? Either answer is fine, just flagging so it isn't missed at release time.
Backfills changelog entries for ten PRs merged since v0.66.1 without a version bump (#465 #486 #487 #488 #490 #492 #493 #494 #495 + #500), attributed to their bump windows (0.67.0 / 0.70.0 / 0.70.1 / 0.71.0), and aligns the version at 0.71.0 as the catch-up release: 0.67.0-0.70.1 were merged to main but never tagged or published, so auto-update users are still on 0.66.1. The v0.71.0 tag + GitHub Release follow.
What
Upload failures against cloud storage (
storage file-upload,storage upload-table, data-app artifact paths -- everything flowing throughKeboolaClient._upload_to_cloud) now surface the provider's short error code and preserve the full provider error body in the DEBUG log:Cloud storage upload failed (HTTP 403, AccessDenied)instead of the previous opaqueCloud storage upload failed (HTTP 403).CLOUD_UPLOAD_ERROR_BODY_LIMIT= 1500 chars) is logged before raising, so--verbosereveals the exact denial detail.Why
On 2026-07-20 file uploads to project 9621 (BigQuery,
connection.keboola.com) started failing with a bareHTTP 403. The actual cause -- a platform-side IAM misconfiguration, with GCS answeringAccessDenied: <service account> does not have storage.objects.create access-- was only visible in the response body that the CLI discarded. Diagnosing it required a temporary code edit. This change makes that class of failure self-explanatory from the CLI output.Safety
Provider error bodies can embed signed URLs, so the full body goes to the DEBUG log only. The user-facing message carries just the short error code, validated against a strict
[A-Za-z0-9._-]{1,64}whitelist (GCS/S3 XML<Code>, Azurex-ms-error-codeheader). A<Code>containing anything else (e.g. injected URL with a signature) is dropped -- covered by a dedicated test.Tests
TestExtractCloudErrorCode: GCS/S3 XML code, Azure header code, unparseable body ->None, charset-whitelist rejection of injected content.TestUploadToCloudErrorDiagnostics: 403 message carries the code while the body detail stays DEBUG-only; 403 without parseable code keeps the plain message; success path unaffected.make checkgreen (4321 passed).No version bump (code-only, rides with the next release).