Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49f99c3d69
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fn supports_stream_options(model_name: &str) -> bool { | ||
| let normalized = model_name.to_ascii_lowercase(); | ||
| !normalized.contains("gemini") |
There was a problem hiding this comment.
supports_stream_options() keys off model_config.model_name, but in this provider that value is the Databricks serving endpoint name: get_endpoint_path() interpolates it directly into /serving-endpoints/{name}/invocations, and fetch_supported_models() returns endpoint.name. In deployments where a Gemini-backed endpoint is aliased to something like prod-chat or assistant-v1, this predicate stays true, Goose still sends stream_options, and the request reproduces the same 400 INVALID_ARGUMENT this patch is meant to eliminate.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| if let Some(text) = &chunk.choices[0].delta.content { | ||
| let (text_content, _thought_signature) = extract_content_and_signature(chunk.choices[0].delta.content.as_ref()); |
There was a problem hiding this comment.
The new array-content path parses thoughtSignature and then drops it: extract_content_and_signature() flattens all parts into one string, and response_to_streaming_message() ignores the returned signature entirely. That makes the first streamed turn succeed, but the assistant Message no longer contains the signature-bearing part, so the next request cannot replay Gemini's saved reasoning state. The native Google formatter in this repo preserves these signatures for follow-up turns/tool loops, so this OpenAI-compatible path will still lose context after the initial response.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 852929dc53
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let status = resp.status(); | ||
| let error_text = resp.text().await.unwrap_or_default(); | ||
|
|
||
| // Parse as JSON if possible to pass to map_http_error_to_provider_error | ||
| let json_payload = serde_json::from_str::<Value>(&error_text).ok(); | ||
| return Err(map_http_error_to_provider_error(status, json_payload)); | ||
| } | ||
| Ok(resp) | ||
| }) | ||
| .await | ||
| .inspect_err(|e| { | ||
| let _ = log.error(e); | ||
| })?; | ||
| .await; |
There was a problem hiding this comment.
stream_options
On Gemini-backed Databricks endpoints this 400 is deterministic, but the request goes through with_retry() before the stream_options fallback runs. I checked crates/goose/src/providers/retry.rs, and should_retry() treats every ProviderError::RequestFailed as retryable, so affected requests will send the same bad payload up to four times with backoff before line 423 finally strips stream_options. That adds several seconds of latency and extra billable calls to every streaming turn on the models this patch is trying to unblock.
Useful? React with 👍 / 👎.
| let text_parts: Vec<_> = parts.iter().filter(|p| p.r#type == "text").collect(); | ||
|
|
||
| let text = text_parts | ||
| .iter() | ||
| .map(|p| p.text.as_str()) | ||
| .collect::<String>(); |
There was a problem hiding this comment.
Gemini's OpenAI-compatible chat completions support include_thoughts, and those thought summaries arrive as content parts rather than reasoning_content. This code concatenates every type == "text" part into MessageContent::text, but ContentPart never records whether a part was a thought, so any thought: true segment will be shown and persisted as normal assistant output instead of hidden MessageContent::Thinking. In thought-enabled sessions that leaks the model's internal reasoning to users.
Useful? React with 👍 / 👎.
DOsinga
added
the
needs_human
michaelneale
left a comment
michaelneale
left a comment
There was a problem hiding this comment.
I think this is fine if clean, as we are the users of this, alex would have tried it with db internally
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71a7300ea8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let signature = text_parts | ||
| .iter() | ||
| .find_map(|p| p.thought_signature.as_ref()) |
There was a problem hiding this comment.
Gemini 3 places the continuation signature on the last part of a non-function response, but extract_content_and_signature() keeps the first thoughtSignature it sees. If a single delta.content array contains multiple text parts, last_signature can be set to a stale value, and the next tool call/follow-up turn will be replayed with the wrong signature. That breaks the continuation contract this patch is trying to restore and can reproduce Gemini's missing/invalid thought_signature 400s on later turns.
Useful? React with 👍 / 👎.
| if let Some(text) = text_content { | ||
| if !text.is_empty() { | ||
| content.push(MessageContent::text(text)); | ||
| content.push(MessageContent::text(&text)); | ||
| } |
There was a problem hiding this comment.
This branch updates last_signature and then downgrades the signed Gemini part to MessageContent::Text, which means the signature is discarded unless a tool call happens later in the same response. In format_messages() assistant text is serialized back as bare text, so a successful first reply from Gemini 3 cannot be appended "including thought signatures" on the next request. For text-only chats that turns this fix into a one-turn success followed by lost reasoning context or another thought_signature validation error on the next turn.
Useful? React with 👍 / 👎.
* main: deps: replace sigstore-verification with sigstore-verify to kill vulns (#8064)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5fbf1f704
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Err(e) if e.to_string().contains("stream_options") => { | ||
| payload.as_object_mut().unwrap().remove("stream_options"); | ||
| self.with_retry(|| async { |
There was a problem hiding this comment.
stream_options fallback
When a Databricks endpoint rejects stream_options, this branch drops the whole object before retrying. On the OpenAI-compatible path, stream_openai_compat() only produces ProviderUsage from streamed usage blocks, and crates/goose/src/agents/agent.rs:1232-1234 only updates session metrics when that usage arrives. As a result, every Gemini/Databricks turn that takes this fallback can succeed while recording no token usage or cost data, silently under-reporting spend for the exact models this patch is meant to unblock.
Useful? React with 👍 / 👎.
| let (text_content, thought_signature) = extract_content_and_signature(chunk.choices[0].delta.content.as_ref()); | ||
|
|
||
| if let Some(sig) = thought_signature { | ||
| last_signature = Some(sig); | ||
| } | ||
|
|
||
| if let Some(text) = text_content { | ||
| if !text.is_empty() { | ||
| content.push(MessageContent::text(text)); | ||
| content.push(MessageContent::text(&text)); |
There was a problem hiding this comment.
id
Databricks/Gemini chunks on this new DeltaContent::Array path can have "id": null (the example in this commit does), and ui/desktop/src/hooks/useChatStream.ts:178-212 only coalesces streamed deltas when consecutive messages share an id. Because these array-content chunks now flow through as separate assistant messages with no fallback id, the desktop stream renders one timestamped bubble per delta instead of extending a single reply, which matches the regression called out in the commit message.
Useful? React with 👍 / 👎.
3 participants
Fixes two major issues blocking usage of gemini models via databricks
stream_optionsisn't supported for gemini models via databrickstextandthoughtSignaturecomes backBefore
and once that is fixed:
After
There is an issue in desktop even after these fixes where multiple timestamps appear interspersed between text chunks when streaming, but hard to tell if that's pre-existing. Open to ideas on a fix.