fix: apply review-recommended fixes for 2.3.0 - #8
Conversation
- README.md: add missing imports to streaming SSE example
(ExpressAgentRunsRequest, WebSearchTool, eventstreaming, all
ResponseXxx event types were used but not imported)
- README.md: fix run_time_ms unit label ("seconds" → "ms")
- retries.py: fix inverted retry_connection_errors flag logic (sync+async)
When True the flag now retries; when False it raises PermanentError
- retries.py: raise instead of returning error response on max_elapsed_time
exceeded so callers are not silently handed a non-2xx httpx.Response
- security.py: add missing f-prefix to three non-interpolating ValueError
messages so the actual sub_type/scheme_type values appear in errors
- sdkconfiguration.py: replace pydantic.Field with dataclasses.field so
retry_config default_factory is evaluated correctly at dataclass init
- MIGRATION.md: add 2.3.0 breaking-changes section covering Python ≥3.10
requirement, Search.count new default of 10, and crawl_timeout int type
- tests/test_research.py: replace misleading if-guard on required
sources field with explicit assert; add len > 0 check to async test
Co-authored-by: Tyler Eastman <tyler5673@users.noreply.github.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if isinstance(exception, TemporaryError): | ||
| return exception.response | ||
|
|
||
| raise |
There was a problem hiding this comment.
Behavioral change that may surprise callers: max_elapsed_time exhaustion now always raises.
Before this change, when all retry time was exhausted and the last error was a TemporaryError (e.g. a 429 or 503 response), the function would silently return that non-2xx httpx.Response. Any caller that checked response.status_code after a retry-exhausted request would see the error without hitting an exception.
After this change, the function unconditionally raises — callers that relied on the old return-on-timeout pattern will now receive an unhandled exception.
The new behavior is more correct (returning a failed response silently is bad), but this is a de-facto behavioral breaking change. Consider:
- Documenting this in the CHANGELOG under 2.3.0 "Changed".
- Making sure the SDK's own exception-handling layer converts the re-raised
TemporaryErrorinto a user-facing typed error (e.g.YouDefaultError) rather than leaking the internalTemporaryErrorclass to callers.
| if isinstance(exception, TemporaryError): | ||
| return exception.response | ||
|
|
||
| raise |
There was a problem hiding this comment.
The async path has the same max_elapsed_time change (see retry_with_backoff_async). Same CHANGELOG note applies.
| except (httpx.NetworkError, httpx.TimeoutException) as exception: | ||
| if retries.config.retry_connection_errors: | ||
| raise | ||
| if not retries.config.retry_connection_errors: |
There was a problem hiding this comment.
Good fix — logic was completely inverted.
Before:
if retry_connection_errors: # True → raise (prevented retry!)
raise
raise PermanentError(...) # False → also prevented retryBoth branches prevented retrying.
After this fix, when retry_connection_errors=True the raw NetworkError/TimeoutException propagates up to the outer retry_with_backoff loop, where it will be caught, the back-off sleep applied, and the request retried. When retry_connection_errors=False it wraps in PermanentError to short-circuit the loop immediately. Correct.
One minor nit: it would help to add a one-line comment explaining why we re-raise the unwrapped exception here vs. wrapping in PermanentError, since the asymmetry is non-obvious:
| if not retries.config.retry_connection_errors: | |
| except (httpx.NetworkError, httpx.TimeoutException) as exception: | |
| if not retries.config.retry_connection_errors: | |
| # Wrap so the outer loop treats this as non-retryable | |
| raise PermanentError(exception) from exception | |
| # Let the raw error propagate so the outer loop retries it | |
| raise |
| if res.output.sources: | ||
| for source in res.output.sources: | ||
| assert source.url is not None | ||
| assert res.output.sources is not None |
There was a problem hiding this comment.
Good strengthening of the assertion. The previous if res.output.sources: guard was effectively a no-op test — if the mock server returned sources=None or [] the test would silently pass without verifying anything. The explicit assert now catches that.
However, these tests depend on the mock server handling POST /v1/research, which it does not — tests/mockserver/internal/handler/generated_handlers.go only registers handlers for /v1/search, /v1/agents/runs, and /v1/contents. A request to /v1/research hits the fallback root handler (404 Not Found), so the SDK raises a YouDefaultError and none of these assertions are ever reached. The tests should either:
- Be skipped with
pytest.mark.skipuntil the mock server is extended, or - Be converted to live-only tests guarded by
pytestmark = pytest.mark.skipif(not os.getenv("YOU_API_KEY_AUTH"), ...)
The mock server binary also needs to be rebuilt for Linux (see the separate comment on the binary).
| assert isinstance(res, ResearchResponse) | ||
| assert res.output is not None | ||
| assert res.output.content is not None | ||
| assert len(res.output.content) > 0 |
There was a problem hiding this comment.
Consistent with the sources fix above — good to add this. The same mock server gap applies here: test_basic_research_async will also fail against the mock server since /v1/research returns 404.
| gen_version: str = __gen_version__ | ||
| user_agent: str = __user_agent__ | ||
| retry_config: OptionalNullable[RetryConfig] = Field(default_factory=lambda: UNSET) | ||
| retry_config: OptionalNullable[RetryConfig] = field(default_factory=lambda: UNSET) |
There was a problem hiding this comment.
Correct fix. SDKConfiguration is a @dataclass, and Pydantic's Field(default_factory=...) only works inside Pydantic BaseModel subclasses. Using it in a plain dataclass would set the default value to the FieldInfo object itself rather than calling the factory, meaning retry_config would always be a FieldInfo rather than UNSET. Switching to dataclasses.field(default_factory=...) is correct.
Worth noting: this is in auto-generated source (# Code generated by Speakeasy). You should verify that the upstream Speakeasy template has been patched so it doesn't regenerate the broken version on the next SDK refresh.
| query_params[header_name] = [value] | ||
| else: | ||
| raise ValueError("sub type {sub_type} not supported") | ||
| raise ValueError(f"sub type {sub_type} not supported") |
There was a problem hiding this comment.
Good fix. All three ValueError messages were missing the f prefix, so they would show the literal string "sub type {sub_type} not supported" instead of embedding the actual value. These are important for debugging unexpected security scheme configurations.
Since this is auto-generated code, same note as sdkconfiguration.py: verify the Speakeasy template is fixed upstream.
| elif isinstance(event_data, ResponseDone): | ||
| print("\n🎉 Response completed!") | ||
| print(f" Runtime: {event_data.response.run_time_ms} seconds") | ||
| print(f" Runtime: {event_data.response.run_time_ms} ms") |
There was a problem hiding this comment.
Good catch. run_time_ms is in milliseconds, not seconds. The unit label "seconds" was misleading — "ms" is correct.
One small style note: the rest of the streaming example output uses emoji (🔍, ✍️, 🌐, 🎉) which is fine for illustration, but the label should match the field name to avoid confusion. run_time_ms → "ms" is the right fix.
|
|
||
| --- | ||
|
|
||
| ## 1.x to 2.0 |
There was a problem hiding this comment.
The 2.3.0 section is useful and well-structured.
Two minor issues to fix:
-
Broken link to
examples/: Line 331 links toexamples/but onlyexamples/api-example-calls.pyexists (not a directory). Consider changing to[examples/api-example-calls.py](examples/api-example-calls.py)or removing the link. -
Missing retries behavioral change: The retries fix (
max_elapsed_timeexhaustion now raises instead of returning the failed response) is a behavior change that existing users relying on response-code checking after retries might hit. Worth a brief note in this section.
| ## 1.x to 2.0 | |
| --- | |
| ## 1.x to 2.0 |
PR Review SummaryThis PR applies a focused set of bug fixes and doc improvements for the 2.3.0 release. The individual changes are all correct and well-targeted. Approved changes
Issues requiring follow-up1. (Must fix) Mock server missing
2. (Must fix) Mock server binary is macOS/ARM64 — won't run on Linux CI
3. (Should fix) Removing the silent 4. (Should fix) Auto-generated files with manual patches
5. (Nice to have) CHANGELOG.md typo on line 176 The "After (2.0)" import example shows 6. (Nice to have) MIGRATION.md broken link Line 331 links to SummaryThe core bug fixes (retries logic, dataclass field, f-string errors) are all correct and needed. The main blockers before merging to release are the mock server gaps — the research tests added to verify 2.3.0 behavior will silently not run until |
Reverts changes to retries.py, security.py, and sdkconfiguration.py. These files are marked "DO NOT EDIT" and tracked in .speakeasy/gen.lock, so any patches here get silently overwritten on the next speakeasy generate run. The fixes should be reported upstream to Speakeasy instead. Specifics: - retries.py conditional restructure was a no-op (logically equivalent) - retries.py error-swallowing removal is a real but edge-case change - security.py f-string prefixes only affect error-path messages - sdkconfiguration.py pydantic.Field vs dataclasses.field has not caused reported issues in practice Keeps the non-generated fixes: README.md imports/label, MIGRATION.md 2.3.0 section, and test_research.py assertion improvements. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Applies non-generated fixes for the 2.3.0 release:
ExpressAgentRunsRequest,WebSearchTool,eventstreaming, allResponse*event types were used but never imported). fixrun_time_msunit label ("seconds" -> "ms")Search.countnew default of 10, andcrawl_timeoutint typelen > 0check to async testDropped from original PR
Changes to speakeasy-generated files (
retries.py,security.py,sdkconfiguration.py) were reverted because:"Code generated by Speakeasy. DO NOT EDIT."and tracked in.speakeasy/gen.lock— patches get silently overwritten on the nextspeakeasy generaterunif flag: raise->if not flag: PermanentError) was logically equivalent — not an actual bug fixpydantic.Field->dataclasses.fieldfix are real but edge-case issues that should be reported upstream to Speakeasy so they're fixed in the generator itselfTest plan
NameErroron missing importsrelease/2.3.0pytest tests/test_research.pyagainst live API to validate stronger assertions