Skip to content

Python SDK 2.5.0 (research background mode)#23

Closed
tyler5673 wants to merge 6 commits into
mainfrom
release/2.5.0
Closed

Python SDK 2.5.0 (research background mode)#23
tyler5673 wants to merge 6 commits into
mainfrom
release/2.5.0

Conversation

@tyler5673

@tyler5673 tyler5673 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Python SDK 2.5.0 release introducing background-mode research: async task submission, polling, and streaming for long-running research tasks. Also adds the new frontier research effort tier for highest-quality, long-running research.

New Features

Method Description
you.research(background=True) Queues research tasks asynchronously, returns TaskResponse with task_id and stream_url instead of blocking for inline ResearchResponse
you.get_research_task(task_id=...) Poll status of a background task via GET /v1/research/{task_id}, returns TaskDetail with result populated on completion
you.stream_research_task(task_id=..., from_id=0) Stream real-time SSE events via GET /v1/research/{task_id}/stream. Terminal event names: response.done, complete, completed (success); error, failed, cancelled (failure)
ResearchEffort.FRONTIER New highest-quality research effort tier. Runs over longer durations (up to 4 hours) with improved quality and accuracy. Only works with the task-based API (background=true); sending frontier without background=true returns a 422.

Convenience Helpers (youdotcom.research_helpers)

Hand-maintained, regen-safe module:

Helper Description
research_background() / _async() Submit and return TaskResponse directly (no Union narrowing)
poll_research_task() / _async() Poll until terminal status (completed, failed, cancelled)
research_and_wait() / _async() Submit + stream SSE events until a terminal event arrives, then fetch the final TaskDetail. If the stream times out or closes without a terminal event, a final get_research_task call resolves the status (returns the detail if completed, raises RuntimeError for terminal non-completed, or TimeoutError if still running)
stream_research() / _async() Tolerant SSE iterator that surfaces undocumented event types as raw dicts instead of crashing (recommended over stream_research_task() for real tasks)

Key Changes

  • you.research() return type widened to Union[ResearchResponse, TaskResponse]. Runtime behavior unchanged when background=False (default). Statically-typed callers accessing res.output may need an isinstance(res, ResearchResponse) narrow to satisfy mypy/pyright.
  • New ResearchEffort.FRONTIER enum value for the highest-quality, longest-running research tier. Documented in CHANGELOG.md, MIGRATION.md, docs/models/researcheffort.md, and docs/sdks/you/README.md.
  • New models: TaskResponse, TaskDetail, ResearchTaskStreamEvent, Event enum, and 8 typed error classes.
  • stream_research recommended over stream_research_task for real tasks — the server emits intermediate workflow events not in the strict Event enum, which causes ResponseValidationError in the generated decoder. The tolerant helper surfaces them as raw dicts instead.

Testing

  • Unit tests with mockserver + httpx MockTransport (160 tests)
  • Live tests pass against production API
  • pylint 10.00/10, mypy clean, Go mockserver builds clean
  • Ponytail and honest code reviews completed; all P2 findings fixed
  • New tests: test_research_frontier_effort_with_background (frontier + background=true returns TaskResponse), test_frontier_without_background_raises_422 (frontier without background returns 422)

See CHANGELOG.md for full release notes and MIGRATION.md for upgrade guidance from 2.4.0.

@tyler5673
tyler5673 marked this pull request as ready for review July 21, 2026 06:22
Comment thread examples/api-example-calls.py Outdated
Comment thread src/youdotcom/research_helpers.py Outdated
Comment thread src/youdotcom/research_helpers.py Outdated
Comment thread tests/test_research_helpers.py Outdated
@youdotcom-oss youdotcom-oss deleted a comment from claude Bot Jul 21, 2026
Comment thread tests/test_research_helpers.py
Comment thread src/youdotcom/research_helpers.py Outdated
@youdotcom-oss youdotcom-oss deleted a comment from claude Bot Jul 21, 2026
Comment thread src/youdotcom/research_helpers.py Outdated
Comment thread src/youdotcom/research_helpers.py Outdated
@youdotcom-oss youdotcom-oss deleted a comment from claude Bot Jul 21, 2026
Comment thread src/youdotcom/research_helpers.py Outdated
Comment thread tests/test_live.py Outdated
Comment thread src/youdotcom/research_helpers.py Outdated
Comment thread src/youdotcom/research_helpers.py Outdated
Comment thread src/youdotcom/research_helpers.py Outdated
Comment thread src/youdotcom/research_helpers.py Outdated
@youdotcom-oss youdotcom-oss deleted a comment from claude Bot Jul 21, 2026
Comment thread src/youdotcom/research_helpers.py Outdated
@youdotcom-oss youdotcom-oss deleted a comment from claude Bot Jul 21, 2026
@youdotcom-oss youdotcom-oss deleted a comment from claude Bot Jul 21, 2026
Comment thread tests/test_live.py
@youdotcom-oss youdotcom-oss deleted a comment from claude Bot Jul 21, 2026
Adds background-mode research (async task submission, polling,
streaming) to the SDK. Adds you.research(background=True),
you.get_research_task(), you.stream_research_task(), and the
hand-maintained youdotcom.research_helpers module. All 2.4.0
features unchanged.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Comment thread src/youdotcom/research_helpers.py Outdated
Comment on lines +404 to +416
if result == "ok":
detail = await client.get_research_task_async(
task_id=task.task_id,
server_url=server_url,
timeout_ms=timeout_ms,
http_headers=http_headers,
)
if detail.status.value != "completed":
raise RuntimeError(
f"research task {task.task_id} stream signalled completion "
f"but GET returned status={detail.status.value}"
)
return detail

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Low severity / robustness (optional). On the success ("ok") path, the SSE stream is abandoned mid-iteration rather than closed deterministically. When _consume() returns after the terminal event, the stream_research_async(...) async generator is suspended at its yield and its finally: await stream.close() only runs when the event loop finalizes the orphaned async-gen (via asyncio's async-gen hooks). That's not a leak in practice, but it's asymmetric with:

  • the sync research_and_wait, which closes the stream in a finally (line 291), and
  • the timeout path here, where wait_for cancellation throws into the async for and runs the finally deterministically.

Consider closing explicitly on the success path for symmetry, e.g. iterate an explicit EventStreamAsync handle (like _open_raw_stream_async) and await stream.close() in a finally, rather than relying on GC finalization. Not blocking.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review Summary

Reviewed the 2.4.0 to 2.5.0 change adding background-mode research (get_research_task, stream_research_task, and the hand-maintained research_helpers module). Focus was on the manually-maintained surface (tests/, CHANGELOG.md, MIGRATION.md, overlays/python_overlay.yaml, pyproject.toml, research_helpers.py).

Overall: strong PR. No correctness bugs that break documented behavior; documentation is accurate and internally consistent; test coverage is excellent.

Code quality & correctness

  • research_helpers.py is well-structured. I verified the tolerant SSE decoder against the EventStream contract: _parse_event builds a {id, event, data, retry} dict and json-dumps it before calling the decoder, so _decode_raw_event reading parsed.get("event") correctly picks up the SSE event: directive (not a nested JSON field). Verified.
  • The stream_research() motivation is legitimate: the generated stream_research_task() validates event against the strict Event enum and would raise ResponseValidationError on undocumented intermediate events; data_required=False correctly lets data-less events (e.g. connected) through.
  • Terminal-state sets (_TERMINAL_TASK_STATUSES, _TERMINAL_STREAM_EVENTS_OK/ERR) match the generated TaskDetailStatus / Event enum values. Verified.
  • The typed-error mapping in _raise_stream_error[_async] faithfully mirrors the generated method 401/403/404/500 branches.
  • One low-severity, optional robustness note left inline: the async research_and_wait_async success path does not close the SSE stream deterministically (relies on async-gen finalization), unlike the sync path and the timeout path.

Test coverage
Comprehensive. test_research_helpers.py (1281 lines) covers sync+async for: research_background (incl. TypeError when the server ignores background=true), poll_research_task (completed / failed / timeout), research_and_wait (terminal ok, error event, timeout raise, timeout GET-fallback-completed, stream-close GET-fallback, stream-close still-running timeout), tolerant streaming with an unknown event name, typed error mapping (401/404), and the raw decoder. test_research.py adds direct SDK-level coverage including the Union[ResearchResponse, TaskResponse] resolution in both directions. Note: several tests depend on the Go mockserver (localhost:18080); I could not execute the suite in this environment, so assertions were verified by reading only.

Documentation

  • CHANGELOG.md, MIGRATION.md, README.md, examples, and tests/README.md are accurate and consistent with the code (method names, model fields, error classes, extra=allow round-tripping of result/input).
  • Version bump to 2.5.0 is consistent across pyproject.toml, _version.py (incl. __user_agent__), and the changelog.

Breaking changes / MIGRATION

  • The you.research() return type widened from ResearchResponse to Union[ResearchResponse, TaskResponse]. This is a type-level breaking change for statically-typed callers; MIGRATION.md documents it clearly (the isinstance/cast note and the no-runtime-break-when-background-omitted note). Handled well.

Security

  • No hardcoded secrets. Live tests read the API key from the environment; test fixtures use obvious placeholder keys (test-api-key) and dummy UUIDs. No injection surface introduced.
  • The overlays/python_overlay.yaml additionalProperties:true injections for TaskDetail.result/input are appropriate and match the extra=allow models; the regen-durability rationale is documented.

Nice work — this is production-ready modulo the optional inline nit.

@youdotcom-oss youdotcom-oss deleted a comment from claude Bot Jul 21, 2026
@youdotcom-oss youdotcom-oss deleted a comment from claude Bot Jul 21, 2026
tyler5673 and others added 2 commits July 21, 2026 16:58
- Remove dead RawStreamEvent.retry field (never read by helpers/tests)
- Extract _resolve_from_final_get[_async] helpers (dedup 4x final-GET logic)
- Table-drive _raise_stream_error[_async] via _STREAM_ERROR_BRANCHES tuple
- Factor _stream_build_kwargs/_stream_hook_ctx shared by sync/async stream-open
- Inline _poll_detail[_async] into their single callers
- P1 fix: replace daemon-thread + consumer.join with single-threaded loop
  using httpx read timeout (no more leaked blocked threads on socket hangs)
- P2 fix: remove dead deadline/time.monotonic() inner check (unreachable -
  httpx.TimeoutException already enforces the timeout)
- P2 fix: remove stream_exc/except BaseException indirection (fossil of
  threaded design; exceptions now propagate naturally)
- P3: fix research_and_wait docstring (one GET -> a GET, second in fallback)
- P3: document retry_config=None divergence from generated stream_research_task
- Add 7 new typed-error tests (403/500/4XX/5XX/async 401/403/500)
- Fix live test TypeError masking: check 'TaskResponse' in str(e) before skip
- Extract _make_wait_handler factory + shared mock stream classes in tests

158/158 tests pass, pylint 10.00/10, mypy clean.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
…fallback, TypeError masking)

- CHANGELOG: fix stream_research_task terminal event list (was missing
  'completed' in ok set and 'failed' in err set; now matches the actual
  _TERMINAL_STREAM_EVENTS_OK/ERR frozensets in research_helpers.py)
- CHANGELOG: document research_and_wait timeout/stream-close fallback GET
  behavior (was described as just 'fetch the final TaskDetail')
- MIGRATION: fix streaming example terminal event set to include 'complete'
  (was only 'response.done' + 'completed')
- MIGRATION: document research_and_wait fallback GET behavior
- examples: fix research_and_wait_example TypeError masking — check
  'TaskResponse' in str(e) before skipping, re-raise real TypeErrors
  (matches the fix applied to test_live.py)
- PR #23 description: fix research_and_wait row (was 'No polling fallback';
  now documents the final-GET fallback) and stream_research_task terminal
  event list

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Comment thread src/youdotcom/research_helpers.py Outdated
Comment on lines +350 to +356
# Use timeout_s as the stream read timeout so iter_bytes raises
# httpx.ReadTimeout deterministically if the server stops sending data.
stream_timeout_ms = int(timeout_s * 1000)
stream = _open_raw_stream(
client, task.task_id, http_headers=http_headers,
server_url=server_url, timeout_ms=stream_timeout_ms,
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

timeout_s means different things in the sync vs. async variant — and the sync docstring is inaccurate.

Here timeout_s is wired in as the per-read (stall) timeout on the SSE stream, so it only fires when the server goes silent for timeout_s seconds. As long as the server keeps sending data (e.g. connected + periodic pings/heartbeats), the sync loop will run well past timeout_s — potentially for the entire task duration, or indefinitely if the server never emits a terminal event but keeps the connection alive.

The async variant (research_and_wait_async) uses asyncio.wait_for(_consume(), timeout=timeout_s), which is a total wall-clock budget. So for the same inputs the two behave differently:

# server streams heartbeats for 60s, then completes; timeout_s=30
research_and_wait(you, timeout_s=30, ...)        # returns completed detail ~60s later
await research_and_wait_async(you, timeout_s=30, ...)  # raises TimeoutError at 30s

The docstring (lines 329-331 / 341-342) says "Maximum seconds to wait for a terminal stream event", which matches the async behavior but not the sync one. Either give the sync path a real total-time budget (e.g. track a deadline and break the loop / close the stream when exceeded), or document that in sync mode timeout_s is a stall timeout, not a total one.

Note the existing test_research_and_wait_timeout_raises_timeout_error only exercises the stall path (_BlockingStream raises ReadTimeout), so this divergence is currently untested.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in commit cc43e45. The sync path now enforces a total wall-clock deadline (deadline = time.monotonic() + timeout_s) inside the stream loop, breaking out when exceeded. Both sync and async now treat timeout_s as a total budget. Added test_research_and_wait_total_deadline_not_stall_timeout which streams non-terminal ping events every 10ms (fast enough to not trip the per-read timeout) and verifies the total deadline fires at timeout_s=0.5s.

@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review: Python SDK 2.5.0 — research background mode

Reviewed with a focus on the manually-maintained surface (tests/, CHANGELOG.md, MIGRATION.md, overlays/, pyproject.toml) and the new hand-maintained research_helpers.py, treating src/youdotcom/** generated files as trusted.

Overall

Solid, well-structured PR. The generated additions (models, errors, get_research_task / stream_research_task) are consistent with existing Speakeasy patterns, and the hand-maintained helper module is careful — it mirrors the generated request setup, reuses the generated typed-error branches, deliberately disables stream-open retries (with a rationale comment), and cleans up streams via try/finally. Version bumps (pyproject.toml, _version.py, uv.lock) are consistent at 2.5.0.

Documentation & breaking changes — accurate ✅

  • CHANGELOG.md and MIGRATION.md both correctly document the one real breaking change: research()'s return type widening from ResearchResponse to Union[ResearchResponse, TaskResponse] (the ResearchResult alias). The MIGRATION guide's type-checker note (isinstance/cast guidance) is a nice touch, and the runtime-unchanged-when-background=False guarantee is verified by test_research_background_false_returns_research_response.
  • overlays/python_overlay.yaml: the additionalProperties: true injections for TaskDetail.result and TaskDetail.input are well-commented and matched by extra="allow" on the Result / TaskDetailInput models, with round-trip assertions in tests. Good regen-durability thinking.
  • README.md error counts (23 → 31, "N of 6" → "N of 8" methods) and method list are updated correctly; tests/README.md reflects the new files.

Test coverage — strong

Both test_research.py (generated-method + mockserver paths) and test_research_helpers.py (MockTransport paths) cover happy paths, all typed error branches (401/403/404/500 + 4XX/5XX fallbacks), poll timeout/failed, stream-close fallbacks, the tolerant-decoder unknown-event case, and sync+async mirrors. test_live.py gates cleanly on YDC_API_KEY/YOU_API_KEY_AUTH and degrades to pytest.skip when background mode isn't server-enabled. No hardcoded secrets (only test-api-key literals).

One issue worth addressing (inline)

  • research_and_wait sync vs. async timeout_s semantics diverge. In the sync path timeout_s is applied as the SSE read/stall timeout, so it only trips on server silence and can run past timeout_s (or hang) while the server keeps streaming; the async path uses asyncio.wait_for for a total wall-clock budget. The docstring ("Maximum seconds to wait for a terminal stream event") describes the async behavior. Recommend either giving the sync path a real total-time deadline or documenting it as a stall timeout. The existing timeout test only covers the stall path, so the divergence is untested. See inline comment.

Minor / non-blocking

  • Nothing else blocking. The async-generator finalization on wait_for cancellation relies on the finally: await stream.close() running — the try/finally is the right pattern here.

Nice work overall; the one timeout-semantics inconsistency is the only thing I'd want resolved (or explicitly documented) before merge.

…atches async)

Addresses PR feedback (claude[bot] review 2026-07-22): sync research_and_wait
used timeout_s only as a per-read stall timeout (httpx read timeout), while
the async variant used asyncio.wait_for as a total wall-clock budget. This
meant a server streaming heartbeats forever would never time out in sync mode.

Fix: add a total deadline check inside the stream loop that breaks and sets
timed_out=True when time.monotonic() >= deadline. Both sync and async now
enforce timeout_s as a total budget.

Added test_research_and_wait_total_deadline_not_stall_timeout: streams
non-terminal 'ping' events every 10ms (fast enough to not trip the per-read
timeout) and verifies the total deadline fires at timeout_s=0.5s.

159/159 tests pass, pylint 10.00/10, mypy clean.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Comment thread src/youdotcom/research_helpers.py
Comment thread src/youdotcom/research_helpers.py
}

// 2) Terminal event — closes the stream. `response.done` is one of the
// four TERMINAL_SSE_EVENTS values that the SDK treats as stream-end.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor: the SDK treats six event names as terminal, not four — _TERMINAL_STREAM_EVENTS_OK = {response.done, complete, completed} plus _TERMINAL_STREAM_EVENTS_ERR = {error, failed, cancelled} in research_helpers.py. The doc-comment on testGetV1ResearchStreamSuccess also lists all six correctly ("Terminal event types: ..."), so this "four" is just an inconsistency here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in commit e000a04. Comment now lists all six terminal event names correctly.

Comment on lines +224 to +229
if detail.status.value != "completed":
raise RuntimeError(
f"research task {task_id} stream signalled completion "
f"but GET returned status={detail.status.value}"
)
return detail

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Coverage gap (minor): the defensive branch where a terminal OK stream event arrives but the follow-up get_research_task reports a non-completed status (this RuntimeError) isn't exercised by any test. The error-event path, timeout path, and stream-close path all have tests; consider adding one where the stream emits response.done but the final GET returns e.g. running/failed to lock in this branch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added in commit e000a04: test_research_and_wait_ok_event_but_get_non_completed_raises — stream emits response.done but the final GET returns running, asserting RuntimeError with match="stream signalled completion but GET returned status=running".

@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review summary — 2.5.0 Research Background Mode

Overall this is a high-quality, well-tested PR. The new background-mode surface (submit -> poll/stream -> result) is coherent, the hand-maintained research_helpers.py is careful and well-documented, and the docs (CHANGELOG / MIGRATION / README / tests README / examples) are accurate and thorough. No correctness or security issues found.

What I verified

  • research_helpers.py internal API usage matches the generated SDK. _stream_build_kwargs / _open_raw_stream mirror the You.stream_research_task _build_request + do_request calls exactly (method, path, headers, security, accept: text/event-stream), and _STREAM_ERROR_BRANCHES reproduces the generated 401/403/404/500 -> 4XX/5XX -> default error ladder. The documented divergence (retry_config=None to avoid silently reopening a half-consumed stream) is sensible.
  • Tolerant decoder is correct. SSE framing is handled upstream in eventstreaming._parse_event, which re-serializes each event as {id, event, data} before calling the decoder, so _decode_raw_event reading the event/data keys is right, and data_required=False lets data-less workflow events through.
  • The strict-decoder rationale in MIGRATION.md is accurate. ResearchTaskStreamEvent.event is a required 7-value Event enum with required id/data, so intermediate workflow events (response.created, etc.) genuinely raise ResponseValidationError in the generated stream_research_task(). Recommending stream_research() is justified.
  • Overlay is well-reasoned. additionalProperties: true on TaskDetail.result/input keeps the regenerated empty Result/TaskDetailInput models (extra=allow) round-tripping the full payloads; the comments explain the regen-durability intent.
  • Breaking change is documented correctly. The research() return widening to Union[ResearchResponse, TaskResponse] (ResearchResult) is runtime-compatible for existing callers, and the MIGRATION.md type-checker note (isinstance/cast) is the right guidance. Version bumps (pyproject.toml, _version.py, user-agent) are consistent.

Test coverage

Strong. test_research_helpers.py covers sync+async for all helpers, terminal-OK / terminal-error / read-timeout / stream-close-still-running paths, the typed-error ladder, and the TypeError fallback when the server ignores background=true. test_research.py and the Go mock handlers cover the generated methods directly, and test_live.py guards live paths behind an API key and skips gracefully when background mode is off.

Minor items (non-blocking) — see inline comments

  1. Go mock comment says four terminal events but the SDK treats six.
  2. Untested defensive branch: terminal-OK stream event but the final GET reports non-completed (RuntimeError in _resolve_from_final_get).
  3. Test-fidelity nit: test_research_and_wait_timeout_raises_timeout_error uses _BlockingStream that raises httpx.ReadTimeout unconditionally, so it exercises the ReadTimeout-handling path but does not actually confirm the SDK applies timeout_s as the stream read timeout.

Nice work — approve once the minor notes are considered.

@youdotcom-oss youdotcom-oss deleted a comment from claude Bot Jul 22, 2026
…t-non-completed test

Addresses PR feedback (claude[bot] review 2026-07-22T00:57):
- Fix Go mockserver comment: said 'four' terminal events but SDK treats six
  (OK: response.done, complete, completed; ERROR: error, failed, cancelled)
- Add test_research_and_wait_ok_event_but_get_non_completed_raises: covers
  the defensive branch where stream emits response.done but the follow-up
  GET returns non-completed status (RuntimeError path in _resolve_from_final_get)

160/160 tests pass, pylint 10.00/10, mypy clean, Go builds clean.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Comment on lines +217 to +229
if result == "ok":
detail = client.get_research_task(
task_id=task_id,
server_url=server_url,
timeout_ms=timeout_ms,
http_headers=http_headers,
)
if detail.status.value != "completed":
raise RuntimeError(
f"research task {task_id} stream signalled completion "
f"but GET returned status={detail.status.value}"
)
return detail

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Robustness: single GET after a terminal-OK stream event can race server-side consistency.

When the SSE stream emits a terminal-OK event (response.done/complete/completed), this path issues exactly one get_research_task, and if that GET returns anything other than completed it raises RuntimeError. If the task store is even slightly behind the SSE emitter (read-replica lag, or the stream fires response.done before the status row is committed), a task that actually succeeded will surface to the caller as a hard RuntimeError.

The stream event itself already carries status: "completed" in its data, so you have a more authoritative signal than the follow-up GET. Consider either trusting the stream's terminal-OK signal, or doing a short bounded re-poll (a couple of interval_s iterations) before raising, so normal replication timing doesn't turn a successful run into a spurious failure. Same pattern in _resolve_from_final_get_async (lines 271–283).

This is deliberate/defensive per the docstring and the test, so not a blocker — flagging it as a real-world reliability edge worth reconsidering.

@claude

claude Bot commented Jul 22, 2026

Copy link
Copy Markdown

PR Review: Research Background Mode (v2.5.0)

Reviewed with focus on the manually-maintained surface (tests/, CHANGELOG.md, MIGRATION.md, overlays/python_overlay.yaml, research_helpers.py, pyproject.toml). Overall this is a high-quality, well-documented change. The hand-maintained research_helpers.py faithfully mirrors the generated stream_research_task request setup (headers, hook context, error-branch mapping) while swapping in a tolerant decoder, and it's backed by an unusually thorough test suite.

Correctness ✅

  • _decode_raw_event correctly reads id/event/data — verified against eventstreaming._parse_event, which re-serializes {id, event, data} into the JSON string handed to the decoder, so the SSE event: name is preserved (not read from the payload body).
  • Sync research_and_wait uses both a per-read timeout and an in-loop wall-clock deadline; async uses asyncio.wait_for. Both bound total wait correctly, and the timeout / stream-close / error-event fallbacks are all exercised by tests.
  • Typed error mapping (401/403/404/500 + 4XX/5XX fallback) matches the generated method and is tested sync + async.
  • background=True correctly widens the return to Union[ResearchResponse, TaskResponse] via the ResearchResult alias; research_background narrows it with a clear TypeError.
  • All imported symbols (_build_request, do_request, match_status_codes, stream_to_text[_async], get_security_from_env, model/error exports) exist and are exported.

Test coverage ✅

Excellent. test_research_helpers.py covers background submit, poll (success/timeout/failed), research_and_wait (terminal-OK, error event, ok-but-GET-non-completed, stall timeout, total-deadline, stream-close), tolerant decoder + unknown event names, and the full typed-error matrix — all mirrored async. test_research.py adds direct SDK-method coverage, and test_live.py gates cleanly on background-mode availability. Go mockserver handlers for the two new GET routes are added and wired into generated_handlers.go.

Documentation ✅

CHANGELOG, MIGRATION (including the type-checker widening note and the strict-decoder caveat), README error tables (23→31 / "of 6"→"of 8"), examples/api-example-calls.py, and tests/README.md are all accurate and consistent with the code. The overlay comments clearly explain the additionalProperties: true injection for TaskDetail.result/input.

Breaking changes ✅

The research() return-type widening is a type-level-only break and is documented in MIGRATION.md with a concrete mitigation (isinstance narrow / cast). Runtime behavior for existing background-less calls is unchanged.

Security ✅

No hardcoded secrets (only test-api-key/bad-key fixtures), no injection vectors. _open_raw_stream intentionally disables retries so a transient 5xx can't silently reopen a half-consumed stream — good call, and documented inline.

One item to consider (non-blocking)

  • Left an inline comment on _resolve_from_final_get: after a terminal-OK stream event, a single follow-up GET is issued and any non-completed status becomes a RuntimeError. Under read-your-writes lag this can turn a genuinely successful task into a spurious failure. The stream event already carries status: "completed", so trusting it or a short bounded re-poll before raising would be more robust. Same pattern in the async variant.

Nice work — the hand-maintained helper and its test coverage are exemplary given the Speakeasy-regen constraints.

Add ResearchEffort.FRONTIER for the highest-quality, longest-running
research tasks (up to 4 hours). Frontier only works with background=true;
sending it without returns 422. The enum value is injected via overlay
(overlays/python_overlay.yaml) so it survives Speakeasy regenerations.

- Update docstrings in researcheffort.py, researchop.py, sdk.py
- Update CHANGELOG.md, MIGRATION.md, docs/models/researcheffort.md,
  docs/sdks/you/README.md
- Add tests: frontier+background happy path, frontier-without-background 422
- Add research_stream_example() to examples/api-example-calls.py
- Update PR #23 description

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
@tyler5673

Copy link
Copy Markdown
Contributor Author

Closing to open a fresh PR with a clean review surface (no stale comments).

@tyler5673 tyler5673 closed this Jul 23, 2026
Comment on lines +234 to +256
# result is None: timeout or stream close without terminal event
detail = client.get_research_task(
task_id=task_id,
server_url=server_url,
timeout_ms=timeout_ms,
http_headers=http_headers,
)
status = detail.status.value
if status == "completed":
return detail
if status in _TERMINAL_TASK_STATUSES:
raise RuntimeError(
f"research task {task_id} ended in non-completed state: {status}"
)
if timed_out:
raise TimeoutError(
f"research task {task_id} did not complete within {timeout_s}s "
f"(status: {status})"
)
raise TimeoutError(
f"research task {task_id} stream closed without terminal event "
f"and task is still {status}"
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Robustness concern for long-running (frontier) tasks: research_and_wait opens a single SSE connection with no reconnection. If an intermediary (LB / proxy) drops the connection before a terminal event — which is likely on multi-hour frontier tasks, since many proxies cap connection lifetime well below 4h — the for loop ends normally (result=None, timed_out=False) and this branch raises TimeoutError("stream closed without terminal event and task is still running") immediately, even though the caller's timeout_s budget (e.g. 14400s) is nowhere near exhausted and the task is healthy.

The server exposes from_id precisely to support resuming a dropped stream (and stream_research already threads it through), but research_and_wait never reconnects. Net effect: the helper the docs recommend for the longest tasks is the one most exposed to spurious timeouts.

Two options worth considering:

  1. On a non-timeout stream close with a still-running task and remaining budget, reconnect from the last-seen id (from_id) instead of raising, or
  2. Fall back to poll_research_task for the remaining budget rather than raising.

At minimum, steer frontier / very-long tasks toward poll_research_task in the docs (see CHANGELOG.md:24). This is currently tested as intended behavior (test_research_and_wait_stream_close_task_running_raises_timeout), so if it's a deliberate choice it's worth a note in the docstring that streaming is not resilient across connection drops.

Comment thread CHANGELOG.md
you,
input="Evaluate the measurable global-health impact of the Gates Foundation",
research_effort=ResearchEffort.FRONTIER,
timeout_s=14400, # frontier tasks can run up to 4 hours

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This example recommends research_and_wait (streaming-based) for a frontier task with timeout_s=14400 (4h). research_and_wait holds a single SSE connection open with no reconnection, so a mid-stream connection drop over that window raises a spurious TimeoutError even though the task is fine (see comment on research_helpers.py). For multi-hour tasks, poll_research_task (discrete GETs) is more resilient. Consider making the headline frontier example use poll_research_task, matching what MIGRATION.md:38 already shows as the "manual polling" path.

@claude

claude Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review: Python SDK 2.5.0 (research background mode)

Reviewed the manually-maintained surface (helpers, tests, CHANGELOG/MIGRATION, overlay, version/deps). This is a well-executed, thorough PR — the hand-written research_helpers.py is careful about stream lifecycle/cleanup, error mapping mirrors the generated methods, and the test suite is genuinely comprehensive (sync + async, terminal/error/timeout/stream-close fallbacks, typed-error branches, and a live suite gated on YDC_API_KEY).

Correctness & consistency

  • Version bump consistent across pyproject.toml, _version.py (incl. user_agent), and uv.lock to 2.5.0.
  • research() return widened to Union[ResearchResponse, TaskResponse] (ResearchResult alias); members are field-disjoint (output required vs. 5 required task fields), so pydantic smart-union discrimination is sound — both directions are tested.
  • Overlay correctly injects frontier into the ResearchEffort enum + description, and additionalProperties true on TaskDetail.result/input to match the extra=allow models. Model/error init.py exports (static + dynamic import maps) are complete.
  • Helper util imports all resolve; EventStream.close()/EventStreamAsync.close() usage matches the actual API (the async try/finally correctly replaces the previously-broken aclosing/aclose path).

Test coverage
Strong. Minor non-blocking gaps: no async analogue of test_research_and_wait_total_deadline_not_stall_timeout (async relies on asyncio.wait_for, so implicitly covered); no assertion that research_and_wait forwards server_url/timeout_ms/http_headers to sub-requests.

Docs ✅ (one concern)
CHANGELOG/MIGRATION are detailed and accurate, including the honest note that the generated stream_research_task() raises ResponseValidationError on undocumented intermediate events and that stream_research() is preferred. USAGE.md is Speakeasy-generated (search-only quickstart) and correctly untouched.

Main finding (non-blocking, robustness)
research_and_wait opens a single SSE connection with no reconnection. On a premature connection close before a terminal event it raises TimeoutError immediately, regardless of remaining timeout_s budget. Fine for short tasks, but the docs headline research_and_wait for frontier tasks with timeout_s=14400 (4h) — exactly the case most likely to hit an intermediary connection drop, producing a spurious timeout on a healthy task. The server supports from_id resume (and stream_research threads it through) but the helper never reconnects. See inline comments on research_helpers.py and CHANGELOG.md line 24. Recommendation: steer frontier/very-long tasks to poll_research_task in the primary example, and/or add a reconnection/poll fallback.

Security
No hardcoded production secrets (only test-api-key fixtures), no injection surface, security still sourced from sdk_configuration.security/env in the hand-written stream path.

Verdict: Approve pending consideration of the frontier streaming-resilience concern (docs tweak is the low-effort fix).

tyler5673 added a commit that referenced this pull request Jul 23, 2026
…rs (#25)

* feat: Python SDK 2.5.0 — Research Background Mode

Adds background-mode research (async task submission, polling,
streaming) to the SDK. Adds you.research(background=True),
you.get_research_task(), you.stream_research_task(), and the
hand-maintained youdotcom.research_helpers module. All 2.4.0
features unchanged.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* refactor: ponytail shrinks + P2 fixes for research_helpers

- Remove dead RawStreamEvent.retry field (never read by helpers/tests)
- Extract _resolve_from_final_get[_async] helpers (dedup 4x final-GET logic)
- Table-drive _raise_stream_error[_async] via _STREAM_ERROR_BRANCHES tuple
- Factor _stream_build_kwargs/_stream_hook_ctx shared by sync/async stream-open
- Inline _poll_detail[_async] into their single callers
- P1 fix: replace daemon-thread + consumer.join with single-threaded loop
  using httpx read timeout (no more leaked blocked threads on socket hangs)
- P2 fix: remove dead deadline/time.monotonic() inner check (unreachable -
  httpx.TimeoutException already enforces the timeout)
- P2 fix: remove stream_exc/except BaseException indirection (fossil of
  threaded design; exceptions now propagate naturally)
- P3: fix research_and_wait docstring (one GET -> a GET, second in fallback)
- P3: document retry_config=None divergence from generated stream_research_task
- Add 7 new typed-error tests (403/500/4XX/5XX/async 401/403/500)
- Fix live test TypeError masking: check 'TaskResponse' in str(e) before skip
- Extract _make_wait_handler factory + shared mock stream classes in tests

158/158 tests pass, pylint 10.00/10, mypy clean.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* docs: fix 2.5.0 doc inaccuracies (terminal events, research_and_wait fallback, TypeError masking)

- CHANGELOG: fix stream_research_task terminal event list (was missing
  'completed' in ok set and 'failed' in err set; now matches the actual
  _TERMINAL_STREAM_EVENTS_OK/ERR frozensets in research_helpers.py)
- CHANGELOG: document research_and_wait timeout/stream-close fallback GET
  behavior (was described as just 'fetch the final TaskDetail')
- MIGRATION: fix streaming example terminal event set to include 'complete'
  (was only 'response.done' + 'completed')
- MIGRATION: document research_and_wait fallback GET behavior
- examples: fix research_and_wait_example TypeError masking — check
  'TaskResponse' in str(e) before skipping, re-raise real TypeErrors
  (matches the fix applied to test_live.py)
- PR #23 description: fix research_and_wait row (was 'No polling fallback';
  now documents the final-GET fallback) and stream_research_task terminal
  event list

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* fix: sync research_and_wait now enforces total wall-clock deadline (matches async)

Addresses PR feedback (claude[bot] review 2026-07-22): sync research_and_wait
used timeout_s only as a per-read stall timeout (httpx read timeout), while
the async variant used asyncio.wait_for as a total wall-clock budget. This
meant a server streaming heartbeats forever would never time out in sync mode.

Fix: add a total deadline check inside the stream loop that breaks and sets
timed_out=True when time.monotonic() >= deadline. Both sync and async now
enforce timeout_s as a total budget.

Added test_research_and_wait_total_deadline_not_stall_timeout: streams
non-terminal 'ping' events every 10ms (fast enough to not trip the per-read
timeout) and verifies the total deadline fires at timeout_s=0.5s.

159/159 tests pass, pylint 10.00/10, mypy clean.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* test: fix Go mock comment (four->six terminal events) + add ok-but-get-non-completed test

Addresses PR feedback (claude[bot] review 2026-07-22T00:57):
- Fix Go mockserver comment: said 'four' terminal events but SDK treats six
  (OK: response.done, complete, completed; ERROR: error, failed, cancelled)
- Add test_research_and_wait_ok_event_but_get_non_completed_raises: covers
  the defensive branch where stream emits response.done but the follow-up
  GET returns non-completed status (RuntimeError path in _resolve_from_final_get)

160/160 tests pass, pylint 10.00/10, mypy clean, Go builds clean.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* feat: add frontier research effort tier + streaming example

Add ResearchEffort.FRONTIER for the highest-quality, longest-running
research tasks (up to 4 hours). Frontier only works with background=true;
sending it without returns 422. The enum value is injected via overlay
(overlays/python_overlay.yaml) so it survives Speakeasy regenerations.

- Update docstrings in researcheffort.py, researchop.py, sdk.py
- Update CHANGELOG.md, MIGRATION.md, docs/models/researcheffort.md,
  docs/sdks/you/README.md
- Add tests: frontier+background happy path, frontier-without-background 422
- Add research_stream_example() to examples/api-example-calls.py
- Update PR #23 description

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* feat: auto-adjust research_and_wait timeout for frontier + surface poll defaults

research_and_wait now auto-selects timeout_s based on research_effort
when omitted: 600s (10 min) for standard/deep/exhaustive, 14400s (4 hr)
for frontier. Explicit timeout_s always takes precedence. poll_research_task
cannot auto-detect (receives task_id, not effort) so its defaults
(interval_s=2.0, timeout_s=600.0) are now documented in docstrings and
MIGRATION.md with a per-tier guidance table.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* feat: add FinanceResearchEffort.LITE + fix PR review findings

Add LITE tier to FinanceResearchEffort enum (overlay-injected, same
pattern as ResearchEffort.FRONTIER). Update docstrings, docs, CHANGELOG,
and MIGRATION. Add test_finance_research_lite_effort.

Fix three PR #24 review items:
1. _resolve_from_final_get now re-polls up to 3 times (1s interval) when
   the stream signals OK but GET returns non-completed, tolerating backend
   commit races. Updated test_research_and_wait_ok_event_but_get_non_completed_raises
   and added test_research_and_wait_ok_event_repoll_succeeds.
2. research_and_wait_async now catches httpx.TimeoutException in addition
   to asyncio.TimeoutError, matching sync behavior. Added
   test_async_research_and_wait_httpx_readtimeout_falls_back_to_get.
3. Removed misplaced pylint disable-next=protected-access in stream_research.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* test: add live tests for frontier research + finance lite effort

- test_frontier_background_completes: submits frontier task with
  background=true via research_and_wait, verifies TaskDetail completed.
  Passed live in 607s.
- test_frontier_without_background_raises_422: verifies 422 when frontier
  is sent without background=true. Passed live.
- test_finance_research_lite_effort: verifies LITE returns a quick answer.
  Skips gracefully when server hasn't deployed the tier yet (422).

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* fix: bound async SSE per-read timeout to timeout_s (match sync variant)

research_and_wait_async now passes stream_timeout_ms=int(timeout_s*1000)
when the caller didn't set an explicit timeout_ms, matching the sync
variant. Without this, a caller-set You(timeout_ms=60000) would cap SSE
reads at 60s and cause premature TimeoutError on frontier tasks with
auto timeout_s=14400. Also updated the async docstring to note the
per-read timeout parity.

Addresses PR #24 review comment 3640119913.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* fix: distinguish early-break from exhausted re-poll in _resolve_from_final_get

When the re-poll loop hits a terminal non-completed status (failed/
cancelled) on the first GET, raise immediately with a clear message
instead of falling through to the misleading after-N-attempts error.
Added test_research_and_wait_ok_event_but_get_failed_raises_immediately
to cover the early-break branch.

Addresses PR #24 review comments 3640244682 and 3640245293.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* fix: align sync/async stream timeout_ms handling + add async repoll tests

Sync research_and_wait now honors caller timeout_ms for the stream read
when provided (matching async), falling back to timeout_s*1000 when not.
Updated both docstrings to accurately describe the shared behavior.

Added async mirrors for repoll-succeeds and ok-event-but-get-failed
tests, closing the coverage gap flagged in review.

Addresses PR #24 review comments 3640262585 and 3640262855 (the latter
was already fixed in 039fee1 but commented on an older revision).

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* fix: stream-open failure falls back to polling + cap per-read timeout at timeout_s

1. research_and_wait[_async] now catches stream-open failures (transient
   500, 404 on just-created task, etc.) and falls back to poll_research_task
   so a successfully-submitted task is never abandoned.
2. SSE per-read timeout is now capped at timeout_s*1000 in both variants,
   preventing a caller-set timeout_ms larger than timeout_s from causing
   the sync wall-clock to exceed the budget.
3. Added test_async_frontier_auto_timeout_completes_without_premature_timeout
   to mirror the sync frontier auto-timeout coverage.

Addresses PR #24 review comments 3640308132, 3640308929, 3640309427.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* fix: sync research_and_wait falls back to polling on mid-stream errors

The sync variant previously only caught httpx.TimeoutException during
stream iteration, letting non-timeout errors (RemoteProtocolError,
ConnectError on dropped connections) propagate unrecoverably. Now
catches Exception and falls back to poll_research_task, matching the
async variant's behavior and the stream-open failure fallback.

Addresses PR #24 review comment 3640374576.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* fix: narrow fallback to httpx.TransportError + decouple SSE per-read from timeout_ms

- Change `except Exception` to `except httpx.TransportError` for both
  stream-open and mid-stream fallbacks (sync + async). Typed auth errors
  (401/403/404/500 from _raise_stream_error) now propagate to the caller
  instead of being swallowed and masked by a different error type from
  poll_research_task.
- Decouple SSE per-read timeout from caller's timeout_ms. Always use
  int(timeout_s * 1000) for stream reads. Previously, a caller with
  timeout_ms=30000 on a frontier task (timeout_s=14400) would get a
  misleading "did not complete within 14400s" after only 30s.
- Add 5 new tests: stream-open TransportError -> poll succeeds (sync +
  async), stream-open 401 -> typed error propagates (sync + async),
  mid-stream TransportError -> poll succeeds (sync).

Addresses PR #24 review comments 3640425979 and 3640426348.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* fix: close stream before fallback poll in mid-stream TransportError handler

The `return poll_research_task(...)` inside the `try` block meant
`finally: stream.close()` only ran after the entire poll loop completed,
keeping the failed SSE connection open during polling. Now calling
`stream.close()` explicitly before the return so cleanup happens first.

Addresses PR #24 review comment 3640517073.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* docs: fix sync docstring to drop stale timeout_ms parenthetical

The SSE per-read timeout was decoupled from timeout_ms in a previous
commit but the sync docstring still mentioned "(or the caller's
timeout_ms when provided)". Now matches the actual behavior and the
async variant's docstring.

Addresses PR #24 review comment 3640569229.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* ci: add Droid Auto Review workflow for automated PR reviews

Triggers on PR open/synchronize/reopen for internal contributors.
External contributors can be reviewed via workflow_dispatch.
Requires FACTORY_API_KEY secret and Factory Droid GitHub App installed.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* ci: remove Claude Code review workflow in favor of Droid Auto Review

The droid-review.yml workflow now handles automated PR reviews.
Removes the claude.yml workflow that used anthropics/claude-code-action.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* fix: sync Speakeasy version files to 2.5.0

gen.yaml python.version and gen.lock releaseVersion were left at 2.4.0
while pyproject.toml and _version.py were already 2.5.0. Without this
sync, the next speakeasy run would silently revert the version bump.

Addresses PR #25 review comment 3640806529.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* chore: remove .speakeasy config and generate-sdk workflow

No longer using Speakeasy for SDK generation. Removes:
- .speakeasy/ (gen.yaml, gen.lock, workflow.yaml, workflow.lock, etc.)
- .github/workflows/generate-sdk.yml (depended on speakeasy bump/run)

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* fix: address Droid Auto Review feedback (P0/P1/P2)

- P0: Fix actions/checkout@v7 -> @v4 (v7 not published)
- P1: Fix broken link in README.md (models/.md -> researchtaskstreamevent.md)
- P1: Make MIGRATION.md code blocks self-contained with imports + You()
- P2: Add TODO comment for curl|sh integrity verification in workflow_dispatch

Addresses PR #25 review comments 3641083815, 3641083827, 3641083831, 3641083837.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* docs: fix TaskDetail/TaskResponse timestamp types (date -> datetime)

Generated docs incorrectly linked to Python date objects instead of
datetime. Updated to match the actual SDK types in taskdetail.py and
taskresponse.py.

Addresses PR #25 review comments 3641189866 and 3641189887.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

* docs: add frontier to ResearchRequest research_effort field docs

The researchrequest.md docs listed lite/standard/deep/exhaustive but
omitted frontier (which requires background=true). Added frontier with
the background constraint note.

Addresses PR #25 review comment 3641288647.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>

---------

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Sign up for free to 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.

1 participant