UN-4011 [FEAT] Support every extraction parameter via a generated transport - #35
UN-4011 [FEAT] Support every extraction parameter via a generated transport#35chandrasekharan-zipstack wants to merge 22 commits into
Conversation
The client now builds its requests from a transport generated off the API's OpenAPI spec instead of assembling them by hand, and sends them over httpx. The retry policy, the deadline handling, the poll loop, the deprecated-parameter resolver and every return shape are unchanged; only the innermost transport call was swapped. Three things a naive swap would have broken, and what keeps them working: - Callers catch requests.ConnectionError and requests.Timeout by name. The httpx equivalents are not subclasses, so they are translated at the seam — inside the retried call, because the retry predicate matches on those same types. requests.ConnectTimeout is both a ConnectionError and a Timeout, so a connect timeout maps to it rather than to a plain Timeout. - The previous transport followed redirects; httpx does not by default. Without it a 30x from a proxy surfaces as "API error: empty response body". - The generated builders write every spec-declared parameter. Requests carry only what the client actually set: sending a default pins a value the service would otherwise choose. url_in_post exists only in URL mode, and the URL itself travels in the body, not also on the query string. Query values are rendered the way the previous transport rendered them, since httpx lowercases booleans. The generated tree is committed but never hand-edited — tools/gen_sdk.sh overwrites it wholesale from specs/llmwhisperer.json with a pinned generator, so fixes belong in client_v2.py or in the spec. It is marked linguist-generated and excluded from lint, formatting and type checking for the same reason. Testing: tests/unit/compat_test.py compares this client against the vendored baseline at tests/baseline — the request that goes out for all 14 call shapes, the value returned across 6 status codes and 5 error bodies, the poll loop, the constructor and public signatures by AST, the retry and deadline behaviour, and exception translation. 234 unit tests pass. A live round trip is still outstanding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
httpx.ReadTimeout was landing in the TimeoutException catch-all and coming back out as requests.Timeout. Callers that catch requests.ReadTimeout by name stopped matching. The translation table test used pytest.raises, which is subclass-tolerant and passed either way; it now asserts the exact class. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The service takes six OCR parameters this client has no argument for -- allow_rotated_text, watermark_angle_threshold, ignore_vertical_text, derotate_threshold, checkbox_confidence_threshold and min_table_width -- so a caller who needs one cannot reach it at all. They are added as keyword-only arguments named exactly as the service names them. Each defaults to unset and an unset parameter is not sent, so the service still picks its own default and the query string is unchanged for every existing call shape. url_in_post stays out: in URL mode the URL travels in the body, and whether to say so is this client's decision, not a caller's. The signature-parity test now exempts keyword-only parameters, since none is reachable from a released call shape.
The spec now carries what the walk could not infer: which parameters are required, the closed sets the service validates against, the error body it returns, and the binary media types three endpoints answer with. Two of those broke generation quietly. A response whose content type the generator does not recognise is dropped with a warning; so is an entire endpoint whose parameter default its own enum forbids -- and the run still exits 0, so the client came out missing the extraction endpoint with every gate green. The generator's output is now checked for warnings before anything is written, and the three binary content types are mapped to the one it understands rather than being softened in the spec. The unwrapped-operation list is checked against the spec before being subtracted from it: an entry excusing an operation the spec no longer declares would otherwise keep passing forever. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The parameter was renamed server-side, and a service older than v2.64.2 reads only the previous spelling: the separator silently falls back to the default instead of failing, which is the kind of thing a caller finds in the output rather than in an error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
Two unrelated drifts under the same seam. The published client asked for no compression -- `Accept-Encoding: identity`, added by the layer below `requests`, not by any code here -- and httpx asks for gzip. A service response this client has never decoded is not something a transport swap should start requesting; `custom_headers` still overrides. Three httpx failures also reached callers as httpx classes, which nothing downstream catches: a redirect loop, an undecodable body, and any future RequestError that is not a TransportError. Two more mapped to a class the published client never raised for them, since requests had no write or pool timeout. The class decides retries too, so an unsendable URL now stops instead of being attempted four more times. Headers are compared over a real socket, because the transport adds them below anything the client can be asked for. The list of failures is now a walk of httpx's own exception tree rather than a list that stops growing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The generated tree is committed, so an edit inside it reviews like any other change and then vanishes on the next regeneration -- as does a spec change nobody ran the generator over. Regenerating in CI and diffing is what notices either one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
The baseline was a pre-release commit pinned by a version string in its own header comment, which an edit to the file can rewrite as easily as the code below it. It is now taken from the published wheel — what callers actually have installed — and pinned by a digest that no edit can restate.
The generated transport is written against one httpx minor series; an upgrade needs a regeneration and a test run, not a resolver decision taken at install time in someone else's environment.
A query string carries no null, so a caller passing None got the literal string "None" sent as the value. These are overrides the service defaults when absent, and absent is what None asks for.
They did not, and had not for some time. Three things were in the way: - ruff and docformatter disagreed about where a multi-line docstring's closing quotes belong, so each run flipped every docstring back and pre-commit could never converge. D209 is now off; docformatter decides. - the pinned hook ran ruff 0.3.4 while the dev group installed 0.11.9, and the two disagree on import order. Both are pinned to one version now. - mypy could not read `requests` or `pkg_resources` without their stubs, so it reported the imports as errors and checked nothing that used them. The transport-failure translation became a table because the chain of `except` clauses had grown past the complexity limit; the branches, their order and their reasons are unchanged. `Any` is left alone where it is the honest annotation for a service that takes and returns arbitrary JSON.
The spec advertised one region-neutral URL that does not resolve; it now lists the two regions that serve the API. Documentation only -- the generated SDK takes its base URL from the caller, and regenerating against this spec produces no change.
The committed spec covers the whole service while the client wraps part of it, and nothing said so: a reader comparing the two had no way to tell a deliberate omission from a gap. Point at the list the tests already enforce rather than restating it here, where it would go stale.
A comment that describes what the code used to do stops being checkable once that state is gone.
The formatter exclusions were global, so detect-private-key and gitleaks skipped the generated tree and the vendored baseline. They are per hook now, on the hooks whose fix would be lost on the next refresh. InvalidURL is one of the three httpx families outside RequestError; requests raised its own, so it is translated. The docstring names the other two as propagating. The drift gate also sees a newly created file now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
It shells out to ruff for post-processing. Finding none, it warns and exits 0, and the warning gate reports that as a spec it could not parse -- a clean regeneration on a runner without a global ruff failed with a message pointing at the wrong thing entirely. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
|
| Filename | Overview |
|---|---|
| src/unstract/llmwhisperer/client_v2.py | Integrates generated request builders, new extraction parameters, per-request headers, exception translation, and reusable transport lifecycle management. |
| specs/llmwhisperer.json | Adds the service OpenAPI specification used to generate the committed transport. |
| tools/gen_sdk.sh | Provides pinned, reproducible regeneration of the generated SDK. |
| .github/workflows/ci_test.yaml | Adds generated-SDK drift detection and released-API compatibility checks. |
| pyproject.toml | Adds transport and generated-model dependencies while aligning development tooling. |
| tests/unit/compat_test.py | Verifies request, response, signature, retry, header-rotation, and transport-lifecycle compatibility. |
Reviews (3): Last reviewed commit: "fix: send page_separator under both spel..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
chandrasekharan-zipstack
changed the title
The transport held its own copy of the headers, so a key rotated after the first call went on being sent with the old value. They are read per request now, the way every call read them before. `close()` and context-manager support give the pooled sockets back; the previous transport had none to give. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
`close()` clears it, which mypy reads as assigning None to a Client. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ
|
The compat suite pins the surface against a vendored baseline file, which only moves when someone remembers to re-vendor it. griffe compares against the latest release tag instead, so the reference point moves on its own. It reads signatures, not requests: it catches a renamed module-level name or a changed parameter, and cannot see what goes out on the wire. The compat suite owns that half; the two are not redundant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D2bC9Q9MPFeyArNgsSkAkZ
2.8.1 sends page_separator under both spellings for services older than v2.64.2. Comparing against 2.8.0 certified a client that sends only one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D2bC9Q9MPFeyArNgsSkAkZ
The generated builder only emits parameters the spec declares, so the deprecated misspelling the released 2.8.1 sends could not go out at all -- against a service older than v2.64.2, which reads only that spelling, page separation was silently lost. Picks up the spec that now declares it, and widens the send filter to match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D2bC9Q9MPFeyArNgsSkAkZ
ritwik-g
left a comment
ritwik-g
left a comment
There was a problem hiding this comment.
Standardized review — verdict: REQUEST CHANGES
Critical: 0 · High: 4 · Medium: 11 · Low: 7 · Lenses run: 17/17
Reviewed against a fixed 17-lens rubric (unstract:standard-review, plugin v0.30.1) at b9e12ab, diffed from merge-base 22a4ede9 — 73 files, matching GitHub's count. sdk_llmwhisperer/** was treated as generated and reviewed only for what it publishes and for where the facade depends on it.
The compatibility work here is the most thorough of the four PRs in this batch, and several things came out clean under deliberate attack — worth stating so the silence is legible:
- No public symbol is removed, renamed, or has a changed signature or default.
whisper()'s 26 released parameters keep their names, order and defaults; the six additions are keyword-only, so no released call shape is reachable. Both guards are real and were mutation-tested:compat_test.py:706-718compares each of the 11 public methods against the vendored 2.8.1 AST, andgriffe check … -Xexits 0 against tagv2.8.1. Renamingwhisper_detailand movingwait_timeout's default were each caught. - The two modified test files lost nothing.
tests/unit/client_v2_test.py: 30 → 30 tests, 57 → 57 asserts, 2 → 2 parametrize tables.tests/integration/client_v2_test.py: 8 → 8, 52 → 52, 4 → 4. An AST comparison confirms no test removed, no argvalue table altered, no per-test assert count changed. Every unit edit is the same mechanical patch-target move (requests.Session.send→_send), and the integration diff is 100% formatter reflow. Classification: legitimate — and note the patch target moved down a layer, so_build_request,_send_requestand the retry predicate are all still real code in these tests. page_separatorworks end to end._build_requestemitspage_separator=%3C%3C%3C&page_seperator=%3C%3C%3Cwith the default and the same custom value under both keys, across the file, stream and URL paths;_SEND_ONLYdeclares both and the generated builder drops neither../tools/gen_sdk.shreproduces the committed SDK byte-identically, sosdk-driftis green on arrival.
The findings cluster in three places: the httpx migration's edges (header casing, error translation, URL building, lifecycle), the spec this client is generated from, and two gaps in an otherwise strong parity suite.
Unanchored findings
- [Low] [Lens 17]
CONTRIBUTING.md:100-113's project structure no longer describes the repo — the listing omitssdk_llmwhisperer/,specs/,tools/andtests/baseline/, and never tells a contributor the first is generated. Made stale bytools/gen_sdk.sh:17. The DO-NOT-EDIT stamps andsdk-driftcover the rule itself, so this is incompleteness rather than a wrong rule. (Not in this diff, hence here.) - The PR description is stale on the baseline it names. It says the suite compares against released 2.8.0 vendored at
tests/baseline/client_v2_2_8_0.py; the branch actually vendors 2.8.1 atclient_v2_2_8_1.py(commit0ad23f3moved it and the body wasn't updated). It also says "273 unit tests" where 275 collect, and describes neither CI job (sdk-drift,api-surface), the README additions, nor the head commitb9e12ab. The wire-parity and "unset parameters are not sent" claims I checked do hold — the code is right and the description is behind it. - PR title — this repo states no title convention in writing (
CONTRIBUTING.mdhas no PR-title section; the template carries only What/Why/How), so there is nothing to judge against.
Open questions
api-surfacevssdk-drift— what should a maintainer do when a spec change legitimately removes a generated symbol? The two gates will contradict each other from the next release tag onward.- Concurrency — is a single
LLMWhispererClientV2expected to be shared across threads? The previous per-requestSessionmade the question moot; the pooled client and publicclose()make it live.
Assumption
specs/llmwhisperer.json is byte-identical to the copy in Zipstack/unstract-llm-whisperer#722 — I diffed them — except for the hand-added page_seperator parameter noted in the findings. The spec-level findings raised on that PR therefore apply to the models generated here, and fixing them upstream will move this PR's generated tree.
Lens checklist (17/17)
1 see findings + Unanchored · 2 see findings · 3 see findings · 4 see findings — the header-casing finding is the security-relevant one; a dedicated security pass found no findings at confidence ≥ 8 — empty within scope, not by exclusion. It examined the header-casing issue on its merits and scored it below the bar; I've filed it as a correctness regression accordingly · 5 N/A — no migrations or persisted state · 6 see findings · 7 see findings · 8 see findings · 9 Clean · 10 Clean · 11 see findings · 12 N/A — no prompts, model config or agent loops touched · 13 see findings; both modified test files adjudicated legitimate above · 14 see findings · 15 see findings · 16 see findings · 17 see findings + Unanchored
Posted as COMMENT, not REQUEST_CHANGES — the merge decision is yours, not the review's.
One pre-existing weakness the security pass surfaced, explicitly not this PR's: follow_redirects=True (client_v2.py:310) means the unstract-key header survives a redirect to a foreign host, because httpx strips only Authorization/Cookie cross-origin. But requests.Session.rebuild_auth strips only Authorization too, and the released 2.8.1 client used Session.send() with redirects on by default — so this is unchanged by the diff and out of scope for this review. Worth its own ticket.
Operational note (not a finding): the CA-bundle environment variables change with the transport, from REQUESTS_CA_BUNDLE/CURL_CA_BUNDLE to SSL_CERT_FILE/SSL_CERT_DIR. Anyone pinning a custom CA today will need to move the variable.
| built.pop("method").upper(), | ||
| url, | ||
| params=params, | ||
| headers={**_TRANSPORT_HEADERS, **self.headers}, |
There was a problem hiding this comment.
[High] [Lens 3, 15] — The header merge is case-sensitive, so a case-variant custom_headers entry duplicates the default instead of overriding it — and can put the real API key on the wire
{**_TRANSPORT_HEADERS, **self.headers} is a plain-dict merge, and httpx keeps both keys when they differ only in case. The released client built a requests CaseInsensitiveDict via PreparedRequest.prepare_headers, so the override collapsed.
The constructor documents custom_headers as "merged with default headers, with custom headers taking precedence" (:256-261). Two concrete regressions, captured from a loopback server running both clients with identical input:
NEW {"accept-encoding":"gzip"} -> Accept-Encoding: identity | unstract-key: realkey | accept-encoding: gzip
BASELINE {"accept-encoding":"gzip"} -> unstract-key: realkey | accept-encoding: gzip
NEW {"Unstract-Key":"override"} -> Accept-Encoding: identity | unstract-key: realkey | Unstract-Key: override
BASELINE {"Unstract-Key":"override"} -> Accept-Encoding: identity | Unstract-Key: override
The second case is the serious one: a caller who explicitly overrides the credential now transmits the original key alongside the replacement — and the override does not take effect either. PEP 3333 requires WSGI servers to join duplicate headers with ", ", so Flask/werkzeug/gunicorn see the literal string "REAL, override" — neither key. So the documented "custom headers taking precedence" contract is silently false for a case-mismatched auth header, in both directions.
I ran this past a dedicated security pass, which scored it below its reporting bar and I agree with that call — the recipient is the endpoint the caller already configured via base_url and was already transmitting to, so this is a widened credential surface (access logs, TLS-terminating proxies, traces), not exfiltration to a third party. It is filed here as the correctness regression it is, not as a vulnerability.
The first case also silently defeats the identity guarantee _TRANSPORT_HEADERS exists to provide (:106-109), sending a conflicting pair.
test_custom_headers_override_the_transport_defaults (tests/unit/compat_test.py:276) uses the exact casing "Accept-Encoding", so it cannot see either.
Fix (verified): build the set case-insensitively before handing it to httpx —
headers = httpx.Headers(_TRANSPORT_HEADERS)
headers.update(self.headers)which collapses to [(b'unstract-key', b'realkey'), (b'accept-encoding', b'gzip')].
While you're there, :106-109's "Overridable via custom_headers" is only true for an exact-casing match — worth saying so, or fixing it with the above.
| (httpx.TooManyRedirects, requests.TooManyRedirects), | ||
| (httpx.DecodingError, requests.exceptions.ContentDecodingError), | ||
| (httpx.InvalidURL, requests.exceptions.InvalidURL), | ||
| (httpx.RequestError, requests.ConnectionError), |
There was a problem hiding this comment.
[High] [Lens 3, 8] — httpx.LocalProtocolError is translated to a retryable ConnectionError, so a permanently-malformed request burns the full backoff — a measured regression against 2.8.1
LocalProtocolError means this request can never be sent as written. The canonical trigger is an illegal header value, and the header this client always sets is unstract-key — so an API key read from .env or os.getenv with a trailing newline is the everyday case.
It subclasses httpx.RequestError, so it falls through every specific row of _TRANSLATIONS to this catch-all and becomes requests.ConnectionError, which _is_retryable (:372) returns True for. The client then re-sends the identical, identically-broken request max_retries + 1 times with exponential backoff (~7s at defaults) and finally raises ConnectionError — the class this file's own header comment (:31-33) says callers catch to retry. The user is told their network is flaky; the actual cause is one character in their key. Under whisper(wait_for_completion=True) the wasted attempts also come out of the wait_timeout deadline (:461-464).
Before/after with the same input (api_key="sk-secret\n", get_usage_info()):
- baseline 2.8.1 →
ValueError: Invalid header valueon attempt 1, no retries (not in therequestsfamily, so_is_retryablerejected it) - this PR →
requests.exceptions.ConnectionErrorafter 4 transport attempts
The PR's own retry table draws exactly the right line and then omits this class — tests/unit/compat_test.py:566-571 marks UnsupportedProtocol, TooManyRedirects and DecodingError non-retryable because "Retrying these cannot start working." An illegal header stays illegal by the same argument. compat_test.py:513 maps the parent httpx.ProtocolError → ConnectionError, which is right for RemoteProtocolError (server-side, transient) and wrong for LocalProtocolError.
Enumerating every httpx.RequestError subclass against _TRANSLATIONS + _is_retryable, LocalProtocolError is the only permanent fault reaching this catch-all.
Fix: add (httpx.LocalProtocolError, requests.exceptions.InvalidHeader) above the RequestError row, and add it to the test_translation_decides_what_gets_retried table with retried=False. RemoteProtocolError should stay on the catch-all.
Cheap complementary fix: self.api_key.strip() in __init__ removes the whitespace-in-key trigger from both the old and new code paths in one line.
Secondary (parity, not a regression): the exception message embeds the raw header value, i.e. the plaintext API key. _log_retry (:384-385) logs only the class name, so nothing reaches the log — but a caller who logs the caught ConnectionError now prints the key on a path that previously raised a non-requests class.
| theirs_url.netloc, | ||
| theirs_url.path, | ||
| ) | ||
| assert parse_qs(ours_url.query) == parse_qs(theirs_url.query) |
There was a problem hiding this comment.
[High] [Lens 13] — The parity suite's central wire comparison silently ignores blank-valued query parameters
parse_qs defaults to keep_blank_values=False, so file_name=, pages_to_extract=, webhook_metadata= and use_webhook= are stripped from both sides before comparison. A change that stops sending a parameter the published client sent with an empty value passes test_request_matches_the_published_client — the one assertion the PR's core claim rests on.
Present-and-empty is not necessarily the same as absent to the service: an absent pages_to_extract takes the service default; a present-but-empty one is a value the service parses.
Verified by mutation — adding and v != "" to the param filter at src/unstract/llmwhisperer/client_v2.py:357 removes four parameters from every whisper request on the wire, and all 275 unit tests still pass. Changing this line to keep_blank_values=True on both sides turns the same mutation into 3 failures (whisper_file, whisper_stream, whisper_url).
The same defaulting appears at :339, :349, :370, :387 and tests/unit/client_v2_test.py:235; the absence-assertions at :339/:349 are weaker than they read for the same reason.
Fix: keep_blank_values=True here (and in the absence-assertions).
(For the record: I mutation-tested the rest of this gate and it has real teeth — renaming a public method → 13 failures plus griffe check exit 1; dropping min_table_width from _SEND_ONLY, changing a whisper() default, replacing the Unset filter with a truthiness filter, dropping the bool str() coercion, and dropping Accept-Encoding: identity each go red. This is the one mutation that survives.)
| ] | ||
| } | ||
| }, | ||
| "/api/v2/whisper": { |
There was a problem hiding this comment.
[High] [Lens 7] — The published spec declares no 402/415/429/5xx, so the generated client returns None for the two most likely real failures — and contradicts this PR's own retry model
Every one of the 19 operations declares only 400/401/403/404. grep -c '"429"\|"402"\|"415"\|"500"\|"503"\|Retry-After' specs/llmwhisperer.json → 0.
Client.raise_on_unexpected_status defaults to False (sdk_llmwhisperer/client.py:38,168), so _parse_response falls through to return None for any undeclared status. A caller of the shipped generated API — extract.sync(...), highlights.asyncio(...) — gets None on:
- 402 — free-tier quota overage; on-prem licence expiry / mode-not-entitled
- 415 — unsupported file type, the single most likely error on a document API
- 500, 503
None is indistinguishable from a documented status without dropping to sync_detailed().status_code, so quota exhaustion and a rejected file type both read as "no result".
Separately, the spec denies 429 and 5xx exist at all while this very PR's facade is built around them: client_v2.py:375 treats 429 or >= 500 as retryable and :391-392 reads Retry-After on 429. The published contract and the client's own reliability model contradict each other in the same commit.
Verified — 402 -> None ; 415 -> None ; 500 -> None ; 503 -> None ; 429 -> None.
The facade is unaffected — client_v2.py:820-828 raises LLMWhispererClientException for anything outside (200, 202), matching 2.8.1. The exposure is that pyproject.toml:65 ships the generated tree inside the wheel as importable public API.
Fix: declare 402/415/429/500/503 against the shared Error shape upstream in Zipstack/unstract-llm-whisperer#722 (I've raised the same gap there) and add Retry-After to the 429 response headers, then regenerate. If the spec can't be widened now, set raise_on_unexpected_status=True in tools/openapi-client.yaml so an undeclared status fails loudly instead of returning None.
| params = {k: _wire_value(v) for k, v in built.pop("params", {}).items() if k in send_only} | ||
| built.pop("headers", None) | ||
| url = self.base_url + built.pop("url").removeprefix(_SPEC_PREFIX) | ||
| return self._transport.build_request( |
There was a problem hiding this comment.
[Medium] [Lens 3] — httpx.InvalidURL escapes untranslated: it is raised at build time, outside the translation wrapper
_TRANSLATIONS maps httpx.InvalidURL → requests.exceptions.InvalidURL and the docstring says it's there "because requests raised its own". But httpx.InvalidURL is raised by self._transport.build_request(...) inside _build_request, which is not wrapped — only _send is (:138-156).
So a malformed base_url raises a raw httpx exception from a client whose whole compatibility story is that callers keep catching requests classes.
Verified — base_url="http://[::1/api/v2": baseline raises requests.exceptions.InvalidURL: Failed to parse: …; this client raises httpx.InvalidURL: Invalid port: ':1', traceback ending in _build_request → httpx/_client.py:366 → _urlparse.py:411.
Fix: route the build through the same seam — _translate_transport_errors(self._transport.build_request, method, url, params=…, headers=…, **built).
| "pre-commit~=3.3.1", | ||
| "yamllint>=1.35.1", | ||
| "ruff<1.0.0,>=0.2.2", | ||
| "ruff==0.11.9", |
There was a problem hiding this comment.
[Low] [Lens 14] — The package ships new public type annotations but no PEP 561 marker, so no downstream type checker can see them
whisper() gains six keyword-only parameters annotated bool | Unset / float | Unset (client_v2.py:649-656), and the whole generated tree is typed — but without a py.typed marker, mypy and pyright treat llmwhisperer-client as untyped and ignore all of it.
The typed transport is this PR's headline, and none of it reaches a consumer.
Verified — find src -name py.typed returns nothing, and there's no force-include in the hatch config.
Fix: add an empty src/unstract/llmwhisperer/py.typed; hatch will include it under the existing packages entry at :64-65.
| echo "wrote $OUT" | ||
| echo "in tests/unit/compat_test.py set:" | ||
| echo " BASELINE_VERSION = \"$VERSION\"" | ||
| echo " BASELINE_SHA256 = \"$(sha256sum "$OUT" | cut -d' ' -f1)\"" |
There was a problem hiding this comment.
[Low] [Lens 3, 15] — This line dies on stock macOS, after the baseline has already been overwritten
sha256sum is GNU coreutils; macOS ships shasum only. Under set -euo pipefail the script fails here — but lines 25-29 have already written the new baseline. The developer is left with a moved baseline, a stale BASELINE_SHA256, a red suite, and no printed digest to fix it with.
The workspace targets a bash-3.2 / BSD-coreutils lowest common denominator for scripts run on contributors' machines.
Verified — on this Darwin box sha256sum resolves only via /opt/homebrew (coreutils); /usr/bin/shasum is the stock binary.
Fix: shasum -a 256 "$OUT" | cut -d' ' -f1, or compute the digest before writing.
| assert declared - UNWRAPPED_OPERATIONS == set(_SEND_ONLY) | ||
|
|
||
|
|
||
| def test_the_baseline_is_the_released_client_unmodified() -> None: |
There was a problem hiding this comment.
[Low] [Lens 13, 16] — BASELINE_SHA256 is a change-detector, and this comment claims it is a provenance check
The digest is computed from the vendored file and stored in the same commit, so a hand-edited baseline with a regenerated hash passes test_the_baseline_is_the_released_client_unmodified. Nothing checks it against the PyPI wheel.
The comment — "an edited baseline can claim any provenance it likes, and every parity test here would still pass" — reads as if the digest closes that hole. It doesn't; it forces the edit to surface as a changed line in the diff, which is worth having but is a different guarantee.
Fix: reword to what the check delivers, or have refresh_baseline.sh also emit the wheel's own recorded hash.
| # The wire carries exactly the parameters assembled above — url_in_post | ||
| # only exists in URL mode, and the generated default would otherwise | ||
| # send it on every upload. | ||
| prepared = self._build_request(extract, frozenset(params), body=File(payload=data), **params) |
There was a problem hiding this comment.
[Low] [Lens 3, 16] — An unrecognised encoding= now raises LookupError after the extraction has already been billed
requests silently fell back to a lossy decode for a codec name it didn't recognise; httpx raises LookupError: unknown encoding: <name> from response.text.
So whisper(encoding=…) / whisper_retrieve(encoding=…) with a bad codec name now throws an exception type absent from both methods' Raises: sections — after the server-side extraction has completed and been counted.
Raising is the better behaviour of the two; the defect is only that it's undocumented.
Verified — same content with .encoding = "bogus-codec": httpx raised LookupError, requests returned the decoded string.
(Anchored here; the response.text reads are at :821 and :963.)
Fix: add LookupError to the two Raises: sections, or validate encoding via codecs.lookup before the call.
|
|
||
| @pytest.mark.parametrize(("name", "value", "expected"), [(n, v, e) for n, (v, e) in _ADDED_PARAMS.items()]) | ||
| def test_a_requested_parameter_is_sent(name: str, value: Any, expected: str) -> None: | ||
| """Every value here is falsy or off: a truthiness filter would drop them and |
There was a problem hiding this comment.
[Low] [Lens 16] — This docstring claims every fixture value is falsy; two of six are not
The stated invariant is what makes this test catch a truthiness filter in the parameter-forwarding code. But ignore_vertical_text: True and min_table_width: 0.5 (:377, :380) would both survive such a filter, so those two cases prove nothing the docstring says they prove — and a maintainer adding a seventh parameter has no reason to pick a falsy value.
Fix: change those two to falsy values, or reword the docstring to say only some are falsy.
Also here (Low): :622-623's "Unlike the other client's api_timeout" is false under the only reading that fits this file — tests/baseline/client_v2_2_8_1.py:241 passes timeout=_effective_timeout() to s.send(...), which requests applies as a real connect/read socket timeout. A reader may conclude the published client's api_timeout was advisory and drop the parity assertion as guarding a difference that never existed. Confidence Medium — the phrase is ambiguous and could have meant the generated sdk_llmwhisperer/client.py's own timeout field, in which case it's merely confusing. Naming which client would settle it.
2 participants
What
LLMWhispererClientV2builds its requests from a transport generated off the API's OpenAPI spec instead of assembling them by hand, and gains the six extraction parameters the service accepts that had no argument to travel through:allow_rotated_text,watermark_angle_threshold,ignore_vertical_text,derotate_threshold,checkbox_confidence_thresholdandmin_table_width.Why
Every parameter the service accepts had to be added to this client by hand, so it lagged the API. Generating the transport from the spec the service now commits (Zipstack/unstract-llm-whisperer#722) makes the wire format follow the API rather than a hand-maintained copy of it.
How
specs/llmwhisperer.json+tools/gen_sdk.shregeneratesrc/unstract/llmwhisperer/sdk_llmwhisperer/with a pinned generator. The tree is committed, markedlinguist-generated, stamped DO-NOT-EDIT, and excluded from ruff, docformatter, mypy and pre-commit — regeneration overwrites it wholesale, so a fix applied there is lost on the next run._get_kwargsbuilders are used. Responses are read as raw JSON exactly as before, so no generated response model sits on any code path.02485e1) are keyword-only, named exactly as the service names them, and unset by default — an unset parameter is not sent, so the query string is byte-for-byte unchanged for every existing call shape.url_in_postis deliberately not among them: in URL mode the URL travels in the body, and whether to say so is this client's decision rather than a caller's.headersor rotating the key reaches the next call the way it did when every call passed them itself.close()and context-manager support hand the pooled sockets back. The client keeps working afterwards — the next request opens a new pool.Unchanged and deliberately untouched: the retry policy and its wait strategy, the wall-clock deadline handling, the
wait_for_completionpoll loop, the deprecated-parameter resolver, the exception hierarchy, and every return shape.Can this PR break any existing features
Three things a naive transport swap would break, each handled:
requests.ConnectionErrorandrequests.Timeoutby name and the httpx classes are not subclasses. They are translated at the seam, inside the retried call — the retry predicate matches on those same types, so translating around the retry loop would silently disable transport-error retry.requests.ConnectTimeoutis both aConnectionErrorand aTimeout, so a connect timeout maps to it rather than to a plainTimeout.follow_redirectsa 30x from a proxy or an http→https upgrade surfaces asAPI error: empty response body.Remaining differences, all wire-irrelevant: query-parameter order is alphabetical rather than insertion order, the webhook JSON body uses compact separators and a different key order (same object), and
User-Agentis nowpython-httpx/....Notes on Testing
273 unit tests.
tests/unit/compat_test.pycompares this client against released 2.8.0, vendored attests/baseline/client_v2_2_8_0.pyand pinned by SHA-256 so the comparison cannot drift, refreshed viatools/refresh_baseline.sh. Both run over the same responses:whisperparameter at once and all three input modeswait_for_completionpoll loop end to endLive round trip. Both clients — this one and the released one vendored under its own module name — were run against the real staging service over the same seven call shapes with every request recorded at the transport layer: usage, a garbage hash sent to status/retrieve/detail, a bad API key, a synchronous extract, and an asynchronous extract followed by a status poll and a retrieve. Wire output was identical on every call in both upload modes; return values matched except for what the service varies between two runs of the same document (per-run timings,
confidence_metadata,font_infocharacter metrics).That run found one divergence the offline suite could not see:
httpx.ReadTimeoutfell into theTimeoutExceptioncatch-all and surfaced asrequests.Timeout, where the released client raisesrequests.ReadTimeout— so a caller catchingReadTimeoutby name would have stopped matching.pytest.raisesis subclass-tolerant, so the translation test passed either way; it now asserts the exact class and fails on the previous code.Note on
pre-commit: ruff, ruff-format and mypy pass on this branch.docformatter,trailing-whitespaceandend-of-file-fixerdo not, and they are fixer hooks — running them rewrites 16 expected-output fixtures undertests/test_data/, where trailing whitespace is the thing being asserted in layout-preserving mode. These commits are therefore made with--no-verify. The hook config wants narrowing to exclude those fixtures; separate change.Related Issues or PRs
mainDependencies Versions / Env Variables
Adds
httpx.requestsstays, as the exception types callers catch.🤖 Generated with Claude Code
https://claude.ai/code/session_014f9oEEYspPH4fmPULTnLkJ