Repository navigation
feat(models): let image blocks set OpenAI's image detail level - #14
poshinchen wants to merge 1 commit into
Conversation
Adds an optional detail (low, high, auto) to ImageContent. OpenAIModel, OpenAIResponsesModel (user content and tool results) and the bidi OpenAIRealtimeModel pass it through; payloads without detail are unchanged. Resolves strands-agents#4531
poshinchen
left a comment
poshinchen
left a comment
There was a problem hiding this comment.
Strandly v3-reason (optimized) review
Assessment: approve
Assessment: Approve
A focused, well-scoped feature that threads an optional detail through the OpenAI Chat Completions, Responses, and Realtime providers while leaving every other provider's wire shape unchanged. Default payloads are preserved and the backward-compatible rationale (and the deliberate omission of original) is clearly documented.
Review Categories
- Testing: Strong — covers both Responses image sites, the bidi dict→dataclass round-trip, LiteLLM reuse, and a Bedrock filtering case; uses full-object equality assertions.
- Documentation: Clear user-facing docs with runnable example and TS parity note.
- Minor: Inaccurate
Raises:docstring on the new Responses_format_input_image; consider whetherdetailwarrants client-side validation vs. relying on the type literal; confirm API-review labeling for the new public type.
Nicely done — the empirical token-count table and byte-identical-payload check give good confidence in the change.
| Raises: | ||
| ValueError: If the image size exceeds the maximum allowed size. | ||
| """ | ||
| input_image: dict[str, Any] = { |
There was a problem hiding this comment.
Suggestion
Issue: The _format_input_image docstring says it raises ValueError: If the image size exceeds the maximum allowed size, but the function body does no size validation — it only encodes bytes and conditionally sets detail. This docstring appears copied and is misleading.
Suggestion: Remove the Raises: section (or replace it with an accurate description of any error actually raised, e.g. a KeyError/TypeError if source["bytes"] is absent). Please verify the intended behavior.
| } | ||
| if "detail" in image: | ||
| input_image["detail"] = image["detail"] | ||
| return input_image |
There was a problem hiding this comment.
Suggestion
Issue: The detail value is passed through verbatim to the OpenAI payload without validation. Because ImageContent is a TypedDict, a dict literal with an out-of-range value (e.g. "medium") is not caught at runtime and would reach the API as a request error.
Suggestion: Consider whether a clear client-side error is preferable to a raw API rejection. If relying on the ImageDetail literal for type-checking is the intended contract, that's acceptable — please confirm this is deliberate.
| source: The source containing the image's binary content. | ||
| detail: The resolution the model processes the image at. Used by `OpenAIModel`, `OpenAIResponsesModel`, | ||
| `OpenAIRealtimeModel`, and the providers built on `OpenAIModel`; ignored by other providers. | ||
| """ |
There was a problem hiding this comment.
Suggestion
Issue: This new public API (ImageContent.detail / ImageDetail / ImageBlock.detail) is customer-facing and alters the content-block contract. The PR description doesn't mention an api/needs-review label.
Suggestion: Please confirm the PR carries the api/needs-review label (or equivalent API bar-raiser sign-off) for the new public literal and the optional field, since it extends a widely-used type.
| ) | ||
|
|
||
|
|
||
| def _format_input_image(block: ImageBlock) -> dict[str, Any]: |
There was a problem hiding this comment.
Suggestion
Issue: The two _format_input_image helpers (this one and openai_responses.py) are near-duplicate logic for building an OpenAI input_image part with an optional detail. Duplication risks the two drifting apart over time.
Suggestion: This is acceptable given one operates on an ImageBlock dataclass and the other on an ImageContent dict, but consider a short note or shared helper if a third site appears. Non-blocking.
GitHub Actions
Failure
Documentation Preview FailedThe documentation deployment encountered an error. Please check the deployment logs for more details. |
poshinchen
changed the title
poshinchen
left a comment
poshinchen
left a comment
There was a problem hiding this comment.
Strandly v4-reason (optimized) review — run af1746, Opus 4.8, upgraded harness (read_file + suggestion-aware reward)
Assessment: approve
Assessment: Approve
A clean, well-scoped feature that threads an optional detail through the three OpenAI providers while leaving every other provider's wire shape untouched. Defaults are preserved and verified by tests.
Review Categories
- API Design: Optional
NotRequiredkey +ImageDetailliteral is additive and non-breaking; the PR description documents use cases, signatures, exclusions (original), and the TypeScript follow-up. Theautodefault semantics differ per API but are intentional and documented. - Testing: Strong coverage with full-object assertions across all three providers, the bidi path, and a Bedrock filtering test confirming the key never leaks to Converse. One minor redundancy with an older partial-assertion test.
- Code Quality: Helper extraction (
_format_input_image) reduces duplication cleanly. Minor docstring accuracy nit on raised exceptions.
Nicely documented tradeoffs and real-token verification table make this easy to trust.
| """ | ||
| input_image: dict[str, Any] = { | ||
| "type": "input_image", | ||
| "image_url": _encode_media_to_data_url(image["source"]["bytes"], image["format"], "image"), |
There was a problem hiding this comment.
Suggestion
Issue: _format_input_image is documented as raising ValueError only "If the image size exceeds the maximum allowed size", but it also raises KeyError on an image missing source.bytes or format (no .get() guards). The behavior is unchanged from the inlined code, so this is non-blocking.
Suggestion: Consider noting the required source["bytes"]/format keys in the docstring to match the Realtime formatter, which explicitly validates and documents its byte requirement.
|
|
||
| tru_result = OpenAIResponsesModel._format_request_tool_message(tool_result) | ||
| exp_result = {"type": "function_call_output", "call_id": "c2", "output": [exp_image_part]} | ||
| assert tru_result == exp_result |
There was a problem hiding this comment.
Suggestion
Issue: The new parametrized test_format_request_tool_message_with_image_detail asserts the full result object (great), which largely overlaps with the pre-existing test_format_request_tool_message_with_image that only does assert "image_url" in tru_result["output"][1]. The weaker per-field assertion in the old test now silently covers less than the new one.
Suggestion: Consider folding the no-detail case here (already present as the first param) and removing or tightening the older partial-assertion test to avoid redundant, weaker coverage.
|
|
||
| format: ImageFormat | ||
| source: ImageSource | ||
| detail: NotRequired[ImageDetail] |
There was a problem hiding this comment.
Suggestion
Issue: ImageDetail includes "auto", and the docstring says other providers "ignore it." Since OpenAI's three APIs are the only consumers today, a value like "auto" passed to the Responses/Realtime path is forwarded verbatim, which is correct. No action needed—just confirming the auto semantics differ per API (Chat Completions defaults to it, Responses/Realtime only send when set).
Suggestion: Optionally add a one-line note in the ImageContent.detail docstring clarifying that unset means "provider default" (Chat Completions sends auto; Responses/Realtime send nothing).
|
Live Strandly's actual comments on the original PR 4966 (copied for comparison; mentions escaped and links de-linked so nothing is pinged or cross-referenced) Review (APPROVED):
Inline at
|
Replica of
https://github.com/strands-agents/harness-sdk/pull/4966(URL in backticks to avoid a cross-reference on the original PR). Branches mirror the original PR's base and head commits.The review below was produced by the optimized Strandly reviewer (variant
v3-reason, optimization runaeb3ee, agent model Opus 4.8) during validation on PRs it never saw in training. Compare with the human and Strandly reviews on the original PR.