Repository navigation
docs(bidi): refine model provider guides [replica of harness-sdk#4894] - #27
poshinchen wants to merge 1 commit into
Conversation
poshinchen
left a comment
There was a problem hiding this comment.
Strandly v3-reason (optimized) review
Assessment: comment
Assessment: Comment
This is a solid documentation refactor that standardizes the setup flow (Installation → Credentials → Configuration) across the three bidi providers, trims redundant voice examples, and routes readers to the quickstart, I/O guide, and API references. The structure and tone improvements are clear wins.
Review Categories
- Link integrity: The rewrite shifts to reference-style links and internal anchors (e.g.,
google.mdx#connection-restarts); please confirm all labels resolve and anchors exist. - Content fidelity: Several technical claims were added/changed (us-east-1 region fallback, Gemini 16kHz/24kHz defaults and auto-transcripts, session-resumption fallback) — verify against current implementation.
- Lost detail: The OpenAI table removed
timeout_s,organization, andproject; thetimeout_s60-minute-limit workaround is non-obvious and should remain discoverable. - API surface: Confirm the newly referenced
AudioCapable/get_audio_config()are intentionally public.
Overall a readable, well-organized improvement — the main asks are verification rather than rework.
File-level comments:
site/src/content/docs/user-guide/sdk/bidi/models/index.mdx: Suggestion
Issue: The session resumption link now points to google.mdx#connection-restarts instead of the external Gemini docs, and adds a "falling back to history replay" claim.
Suggestion: Please verify the #connection-restarts anchor actually exists on the Gemini page (the google.mdx diff shows a "Session settings" / connection section but the anchor isn't confirmed in the visible diff), and that the fallback-to-history-replay behavior is accurate for the SDK.
site/src/content/docs/user-guide/sdk/bidi/models/bedrock.mdx: Suggestion
Issue: The Credentials example drops the region argument in favor of a boto_session, and the prose states the region fallback is us-east-1. The removed caution box previously listed the specific supported regions (us-east-1, us-west-2, eu-north-1, ap-northeast-1).
Suggestion: Please verify the [nova-regions] reference link preserves that region list so readers who relied on the explicit callout can still find it. Also confirm the us-east-1 default-region fallback claim matches the model's actual behavior.
site/src/content/docs/user-guide/sdk/bidi/models/google.mdx: Suggestion
Issue: The example no longer passes client_args={"api_key": ...} and instead relies on the GOOGLE_API_KEY env var, which is clearer. The prose states "16 kHz input by default and fixed 24 kHz output" and "The adapter enables transcripts for both user speech and model responses."
Suggestion: Please verify these audio-rate and automatic-transcript claims against the current GoogleGeminiLiveModel defaults so the docs don't drift from implementation.
site/src/content/docs/user-guide/sdk/bidi/models/openai.mdx: Important
Issue: The removed table documented timeout_s (the 60-minute session limit workaround) and organization/project auth params. The new prose directs readers to the "constructor reference" but these fields are no longer described inline.
Suggestion: timeout_s carries non-obvious behavior (no OpenAI warning before the 60-min limit). Please verify this guidance survives somewhere reachable — either keep a short note or confirm the [model-api] reference clearly surfaces it, since dropping it risks users hitting silent session closures.
site/src/content/docs/user-guide/sdk/bidi/models/openai.mdx: Suggestion
Issue: These guides use many reference-style link labels ([nova-sonic], [nova-regions], [boto3-credentials], [credentials-setup], [realtime], [api-keys], [voices], [transcription-model], [model-api], [gemini-live], [client-api], [session-settings], [vad]). The matching URL definitions appear below the truncated portion of each file.
Suggestion: Since the PR mentions link verification was run, please confirm every reference label has a corresponding definition and no dangling labels remain after the rewrite — unresolved reference links render as literal text.
Inline comments (could not anchor to the diff):
site/src/content/docs/user-guide/sdk/bidi/models/index.mdx:108: Important
Issue: A new reference-style link definition [audio-capable] is added here, and the body references [AudioCapable][audio-capable]. Please verify AudioCapable and get_audio_config() are actually part of the public API at strands.bidi.models so the API-doc anchor resolves.
Suggestion: If AudioCapable is a new/public abstraction surfaced to customers, confirm the API reference anchor builds and that this symbol is intentionally public (consider whether it needs API bar-raising review).
Documentation Preview FailedThe documentation deployment encountered an error. Please check the deployment logs for more details. |
Replica of
https://github.com/strands-agents/harness-sdk/pull/4894(URL in backticks to avoid a cross-reference on the original PR) for reviewer evaluation. 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.