From 21abbe569196d14cf74f615bdd44c9f65b3e3401 Mon Sep 17 00:00:00 2001 From: Artem <91075334+Mffff4@users.noreply.github.com> Date: Thu, 19 Mar 2026 07:26:55 +0300 Subject: [PATCH] fix: address Copilot PR review feedback - Fix empty text part for assistant messages with tool calls (curate_contents could drop entire model turn) - Propagate cache_read/creation_input_tokens in complete_with_tools - Log warning on save_credential failure instead of silently ignoring - Fix doc comment to mention underscore in header name pattern - Handle gemini-oauth (hyphen variant) in setup wizard display - Fix docs: thinkingConfig uses thinkingBudget/thinkingLevel, not includeThoughts --- FEATURE_PARITY.md | 2 +- docs/LLM_PROVIDERS.md | 2 +- src/llm/gemini_oauth.rs | 23 ++++++++++++++++++----- src/setup/wizard.rs | 5 +++-- 4 files changed, 23 insertions(+), 9 deletions(-) diff --git a/FEATURE_PARITY.md b/FEATURE_PARITY.md index 3c2c21e7..d932e9ad 100644 --- a/FEATURE_PARITY.md +++ b/FEATURE_PARITY.md @@ -205,7 +205,7 @@ This document tracks feature parity between IronClaw (Rust implementation) and O | Skills (modular capabilities) | ✅ | ✅ | Prompt-based skills with trust gating, attenuation, activation criteria, catalog, selector | | Skill routing blocks | ✅ | 🚧 | ActivationCriteria (keywords, patterns, tags) but no "Use when / Don't use when" blocks | | Skill path compaction | ✅ | ❌ | ~ prefix to reduce prompt tokens | -| Thinking modes (off/minimal/low/medium/high/xhigh/adaptive) | ✅ | 🚧 | thinkingConfig for Gemini models (includeThoughts); no per-level control yet | +| Thinking modes (off/minimal/low/medium/high/xhigh/adaptive) | ✅ | 🚧 | thinkingConfig for Gemini models (thinkingBudget/thinkingLevel); no per-level control yet | | Per-model thinkingDefault override | ✅ | ❌ | Override thinking level per model; Anthropic Claude 4.6 defaults to adaptive | | Block-level streaming | ✅ | ❌ | | | Tool-level streaming | ✅ | ❌ | | diff --git a/docs/LLM_PROVIDERS.md b/docs/LLM_PROVIDERS.md index a353dd62..02016890 100644 --- a/docs/LLM_PROVIDERS.md +++ b/docs/LLM_PROVIDERS.md @@ -78,7 +78,7 @@ GEMINI_MODEL=gemini-2.5-flash |---|---|---| | Function calling | ✅ | `functionDeclarations` / `functionCall` / `functionResponse` | | `generationConfig` | ✅ | `temperature`, `maxOutputTokens` passed from request | -| `thinkingConfig` | ✅ | `includeThoughts: true` for `gemini-3`/`thinking` models | +| `thinkingConfig` | ✅ | `thinkingBudget`/`thinkingLevel` for thinking-capable models | | `toolConfig` | ✅ | `functionCallingConfig.mode`: `AUTO`/`ANY`/`NONE` | | SSE streaming | ✅ | Cloud Code API with `streamGenerateContent?alt=sse` | | Token refresh | ✅ | Automatic via refresh token | diff --git a/src/llm/gemini_oauth.rs b/src/llm/gemini_oauth.rs index 522f2569..50f0179c 100644 --- a/src/llm/gemini_oauth.rs +++ b/src/llm/gemini_oauth.rs @@ -89,7 +89,7 @@ fn default_safety_settings() -> Vec { /// Parse `GEMINI_CLI_CUSTOM_HEADERS` env var in format `key:value,key:value`. /// Commas inside values are preserved — splits only on commas followed by a -/// valid HTTP header name pattern (ASCII alphanumeric/hyphen, then `:`). +/// valid HTTP header name pattern (ASCII alphanumeric/hyphen/underscore, then `:`). fn parse_custom_headers() -> std::collections::HashMap { let mut headers = std::collections::HashMap::new(); let env_val = match std::env::var("GEMINI_CLI_CUSTOM_HEADERS") { @@ -397,7 +397,9 @@ impl CredentialManager { if let Some(pid) = self.discover_project_id(&updated.access_token).await { info!(project_id = %pid, "Discovered Cloud Code project"); updated.project_id = Some(pid); - let _ = self.save_credential(&updated).await; + if let Err(e) = self.save_credential(&updated).await { + warn!(error = %e, "Failed to persist discovered project_id to credentials file"); + } } return Ok(updated); } @@ -1617,7 +1619,13 @@ impl GeminiOauthProvider { })); } Role::Assistant => { - let mut parts = vec![serde_json::json!({ "text": msg.content })]; + let mut parts = Vec::new(); + // Only add text part if content is non-empty (assistant messages + // with tool calls often have empty content, and curate_contents + // would drop the entire turn if it sees an empty text part). + if !msg.content.is_empty() { + parts.push(serde_json::json!({ "text": msg.content })); + } if let Some(ref calls) = msg.tool_calls { for call in calls { parts.push(serde_json::json!({ @@ -1628,6 +1636,11 @@ impl GeminiOauthProvider { })); } } + // Fallback: if no parts at all, add empty text to avoid + // sending a turn with zero parts (API rejects it). + if parts.is_empty() { + parts.push(serde_json::json!({ "text": "" })); + } contents.push(serde_json::json!({ "role": "model", "parts": parts @@ -2080,8 +2093,8 @@ impl LlmProvider for GeminiOauthProvider { input_tokens: response.input_tokens, output_tokens: response.output_tokens, tool_calls, - cache_read_input_tokens: 0, - cache_creation_input_tokens: 0, + cache_read_input_tokens: response.cache_read_input_tokens, + cache_creation_input_tokens: response.cache_creation_input_tokens, }) } } diff --git a/src/setup/wizard.rs b/src/setup/wizard.rs index c616fdeb..f0ec28c9 100644 --- a/src/setup/wizard.rs +++ b/src/setup/wizard.rs @@ -1037,7 +1037,7 @@ impl SetupWizard { } else { match current.as_str() { "nearai" => "NEAR AI".to_string(), - "gemini_oauth" => "Gemini API (OAuth)".to_string(), + "gemini_oauth" | "gemini-oauth" => "Gemini API (OAuth)".to_string(), _ => { if let Some(def) = registry.find(¤t) { def.setup @@ -1055,6 +1055,7 @@ impl SetupWizard { let is_known = current == "nearai" || current == "bedrock" || current == "gemini_oauth" + || current == "gemini-oauth" || registry.is_known(¤t); if is_known && confirm("Keep current provider?", true).map_err(SetupError::Io)? { @@ -1062,7 +1063,7 @@ impl SetupWizard { print_info("Keeping existing AWS Bedrock configuration."); return Ok(()); } - if current == "gemini_oauth" { + if current == "gemini_oauth" || current == "gemini-oauth" { print_info("Keeping existing Gemini CLI OAuth configuration."); return Ok(()); }