Skip to content

feat(pipeline-run): surface API HTTP failures as clean errors - #38

Open
arseniy-pplx wants to merge 1 commit into
TangleML:masterfrom
arseniy-pplx:transfer/clean-sdk-http-errors
Open

feat(pipeline-run): surface API HTTP failures as clean errors#38
arseniy-pplx wants to merge 1 commit into
TangleML:masterfrom
arseniy-pplx:transfer/clean-sdk-http-errors

Conversation

@arseniy-pplx

@arseniy-pplxarseniy-pplx commented Jul 20, 2026

Copy link
Copy Markdown

Summary

  • add a shared HTTP-error formatter and surface_http_errors context manager for SDK commands
  • report API status, reason, method, URL and a bounded, whitespace-collapsed response body as one-line command errors, with all redaction running before the body is collapsed and truncated
  • judge credential field names by their tokens rather than by substring containment, so affixed and camelCase credentials (access_token, refresh_token, id_token, sessionToken, accessToken, client_secret, user_credential, AwsAccessKeyId) lose their values while max_tokens, tokenizer, token_count, function_signature, private_key_path and secretaryEmail keep theirs
  • sanitize URLs structurally: strip userinfo, redact presigned/SAS signature parameters (signature, X-Amz-Signature, sig, awsaccesskeyid, googleaccessid) while keeping the non-credential SigV4 set (X-Amz-Algorithm, X-Amz-Date, X-Amz-Expires, X-Amz-SignedHeaders) readable, descend exactly one level into a nested URL, sanitize the fragment the OAuth implicit flow delivers a token in, and fail closed on a malformed URL — including an unparsable port, which urlsplit only rejects once the port is read
  • scrub non-JSON bodies (form-encoded, plain text, HTML, truncated JSON): credential assignments and auth-scheme credentials are cut while the scheme name (Bearer <redacted>) and surrounding prose survive; an explicit scheme (Bearer, Basic, Digest, Negotiate, NTLM, SSWS, JWT, ApiKey, GoogleLogin, AWS4-HMAC-SHA256), matched case-insensitively (bearer, BASIC), always loses the word that follows it regardless of its shape or vocabulary (Bearer abc, Basic secret, Bearer token, Basic Access); a chained run of scheme names (Basic Bearer sk, Bearer Bearer sk, mixed/lowercase, 1–20 deep) is consumed in one bounded match, so a non-overlapping scan cannot stop at a doubled scheme and leave the credential behind it untouched, and whitespace between scheme words, field names and values includes newlines and tabs, so the display-time whitespace collapse cannot create an unredacted scheme–credential adjacency; WWW-Authenticate challenge parameters (realm="api", error="invalid_token", nonce) stay readable by grammar rather than by value judgment, and the length heuristic remains only for the ambiguous scheme words Token/OAuth so Token 12345 expired and token count: 42 survive; a URL or bare user:pass@host reflected in a body is scrubbed structurally so its host and path stay diagnosable
  • walk parsed JSON bodies iteratively under a depth bound that fails closed, so a hostile body cannot exhaust the interpreter stack, and anchor every scan rather than backtracking, so cost stays linear in the body length
  • wrap pipeline-run manager API calls (status, details, cancel, graph-state, logs, list/search, export, submit) and annotation commands while preserving client-level recovery, post-submit run recovery, and fallback behavior
  • report per-run graph-state failures with the same formatted message in each result's error field

Context

Pipeline-run SDK commands currently leak raw requests tracebacks when the API returns a non-2xx response. The command layer already converts PipelineRunError to a clean non-zero exit; this change makes HTTP failures on sdk pipeline-runs commands cross that boundary instead. Client-internal recovery (the 404 run-id -> execution-id fallback, submission-id run recovery after a failed submit) keeps handling the statuses it can; only unrecovered errors are formatted.

Because this puts an attacker-influenced response body on an always-on error path, the redaction helpers are transport-neutral and live in api_transport.py beside the existing header/body redaction rather than being reimplemented against requests. Their names and behavior match the equivalent helpers on the httpx error path so the two cannot drift, while this PR stays scoped to its own SDK error path and is independently mergeable.

Two behavior notes. Downstream hook subclasses: on_poll_error and on_submit_error now receive PipelineRunError (with the original requests.HTTPError chained as __cause__) instead of the raw HTTPError on an HTTP status failure; non-HTTP submit errors reach on_submit_error unchanged. Diagnostics trade-off: a field explicitly named as a credential loses its value regardless of what the value looks like, and the same strict rule applies after an explicit auth scheme in any letter case — credential: invalid renders as credential: <redacted> and Bearer abc / bearer token as Bearer <redacted> / bearer <redacted> — gating on "does this value look opaque?" or on a vocabulary of prose words would let a short or word-like credential through, so prose immediately after a scheme name is conservatively lost (Basic authentication failed renders as Basic <redacted> failed) and only challenge parameters, recognized by their name= grammar, are exempt. In a chained run the scheme words themselves are kept and only the credential after the last explicit scheme is cut (Basic Bearer sk renders as Basic Bearer <redacted>); an ambiguous word such as token inside a chain is treated as the preceding scheme's credential, not as a further scheme, so Bearer token expired still renders as Bearer <redacted> expired.

Testing

  • uv run pytest on Python 3.10, 3.11, 3.12 and 3.13 — green apart from tests/test_api_cli.py::test_official_static_command_without_schema_fails_with_actionable_error, which fails identically on unmodified master (re-verified against current master) and is unrelated to this change
  • uv run pytest tests/test_sdk_http_errors.py — 215 cases: the exact reproductions from review, affixed/camelCase credential fields across five separator spellings, scheme values including short, one-letter, and word-like credentials (Bearer token, Bearer TOKEN, Basic Access, Basic credentials) in lower/mixed-case scheme spellings, chained scheme runs (Bearer Bearer sk, Basic Bearer sk, mixed/lowercase, plain text, HTML and JSON leaves, parametrized 1–20 deep), newline/tab separators between field name, scheme and value, punctuation, HTML bodies and JSON string leaves, WWW-Authenticate challenge parameters that stay readable (including after a chained scheme run), credentials reflected in JSON string leaves and in URLs, presigned-URL fields that keep host and path, over-redaction guards for diagnostic query keys and ambiguous Token/OAuth prose and chains, redaction-before-truncation, 9000-deep JSON nesting, wide bodies, and adversarial single-run inputs (including 60000 repeated scheme words) checked for linear cost
  • uvx ruff check — one finding more than master: the I001 import-sort finding the new test module shares with nearly every existing test module; no other new findings
  • uv lock --check, git diff --check

@arseniy-pplx
arseniy-pplx marked this pull request as ready for review July 20, 2026 18:11
return f"Tangle API request failed: {exc}"
request = response.request
target = (
f"{request.method} {request.url}"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(AI-assisted)

Please redact the request URL and backend detail before rendering this error.

This currently interpolates both request.url and response.text verbatim. For example, formatting a request to https://alice:hunter2@api.test/x?access_token=QUERYSECRET with an error body containing credential=BODYSECRET exposes all three values in the resulting stderr line.

Could we sanitize URL userinfo/credential query parameters and pass structured response bodies through the existing sensitive-key redaction before truncation? Ideally the requests/httpx error paths in #38, #45, and #46 should share transport-neutral redaction helpers so their guarantees do not drift.

@Volv-GVolv-G left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(AI-assisted)

Approved per reviewer direction. The inline credential-redaction feedback remains available for follow-up.

@arseniy-pplx
arseniy-pplxforce-pushed the transfer/clean-sdk-http-errors branch 2 times, most recently from 0f8300e to a90101cCompareJuly 24, 2026 11:45

@Volv-GVolv-G left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(AI-assisted)

Re-approved after validating the force-pushed head a90101cf7990b134a265558b1475dda1a6b25781. The prior credential-redaction concern is addressed, the full PR and force-push delta were re-reviewed, and focused/full tests passed locally.

@arseniy-pplx
arseniy-pplxforce-pushed the transfer/clean-sdk-http-errors branch 2 times, most recently from 15ae2fa to bf85882CompareJuly 29, 2026 11:11
Comment on lines +367 to +374
def _replace_bare_scheme(match: re.Match[str]) -> str:
scheme, value = match.group("scheme"), match.group("value")
if scheme in _AMBIGUOUS_BARE_SCHEMES:
if len(value) < _MIN_AMBIGUOUS_SCHEME_CREDENTIAL_CHARS:
return match.group(0)
elif not _looks_like_opaque_credential(value):
return match.group(0)
return f"{scheme} {_REDACTED}"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(AI-assisted) Explicit auth schemes still pass through _looks_like_opaque_credential(), so short or word-like values such as Bearer abc or Basic secret can be returned unchanged. Once an unambiguous auth scheme matches, please always redact its value, keep prose/shape heuristics only for genuinely ambiguous language, and add short/word-like scheme-value regressions.

@arseniy-pplx
arseniy-pplxforce-pushed the transfer/clean-sdk-http-errors branch 2 times, most recently from f9aeef6 to 3bac859CompareAugust 10, 2026 10:42
Pipeline-run SDK commands leaked raw requests tracebacks whenever the API answered non-2xx. Manager and annotation methods that call the client now run under a shared error-surfacing context manager that re-raises requests.HTTPError as PipelineRunError with the status, reason, method, URL and a single-line, trimmed response body, so sdk pipeline-runs commands (including submit and annotations) exit non-zero with a one-line message. Client-internal recovery such as the 404 run-id to execution-id fallback and post-submit run recovery keeps handling the statuses it can; per-run graph-state failures reuse the formatted message in each result's error field. on_poll_error and on_submit_error hooks now receive PipelineRunError with the original HTTPError chained as __cause__.
Harden the shared redaction so an echoed URL or body cannot leak a credential. Field names are now judged by their tokens rather than by substring containment, so affixed and camelCase credentials (access_token, refresh_token, id_token, sessionToken, accessToken, client_secret, user_credential, AwsAccessKeyId) lose their values while max_tokens, tokenizer, token_count, function_signature, private_key_path and secretaryEmail keep theirs. URL sanitization strips userinfo, redacts presigned/SAS signature parameters while keeping the non-credential SigV4 set (X-Amz-Algorithm, X-Amz-Date, X-Amz-Expires, X-Amz-SignedHeaders) readable, descends exactly one level into a nested URL, sanitizes the fragment the OAuth implicit flow delivers a token in, and fails closed on a malformed URL -- including an unparsable port, which urlsplit only rejects once the port is read. Non-JSON bodies (form-encoded, plain text, HTML, truncated JSON) have credential assignments and auth-scheme credentials cut while the scheme name and the surrounding prose survive; an explicit auth scheme (Bearer, Basic, Digest, Negotiate, NTLM, SSWS, JWT, ApiKey, GoogleLogin, AWS4-HMAC-SHA256), matched case-insensitively, always loses the word that follows it regardless of its shape or vocabulary, a chained run of scheme names (Basic Bearer sk) is consumed in one bounded match so the credential after the last explicit scheme cannot survive a non-overlapping scan, and whitespace between scheme words, field names and values includes newlines and tabs so display-time whitespace collapsing cannot create an unredacted adjacency. WWW-Authenticate challenge parameters (realm, error, nonce) stay readable by grammar, and the length heuristic remains only for the ambiguous scheme words Token and OAuth. A URL or bare user:pass@host reflected in a body is scrubbed structurally so its host and path stay diagnosable. Parsed JSON bodies are walked iteratively under a depth bound that fails closed, so a hostile body cannot exhaust the interpreter stack, and every scan is anchored rather than backtracking, so cost stays linear in the body length. All redaction runs before the body is collapsed to one line and truncated.
@arseniy-pplx
arseniy-pplxforce-pushed the transfer/clean-sdk-http-errors branch from 3bac859 to 2b40461CompareAugust 10, 2026 11:36
Sign up for freeto 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.

2 participants

@arseniy-pplx@Volv-G