Addressing PR comments + fix Google Gemini tool schema compatibility

[Copilot]

• Understand current code flow in tool_notifier.rs and telegram.rs
• Issue 1: Change finish() in tool_notifier.rs to always delete the status message
• Issue 2: Send "⏳ Thinking..." placeholder when verbose is off, pass its MessageId to stream handle
• Update stream handle to reuse placeholder message for first edit
• Handle edge case: delete placeholder if no tokens streamed
• Mark format_final() as dead code with allow attribute
• Build, test, clippy, fmt — all pass

[github-actions]

Claude finished @chinkan's task in 3m 30s —— View job

---

Code Review

Todo List:

• Gather context and understand the PR
• Review all 14 changed files
• Post comprehensive review feedback

---

Summary

The PR's stated fix — replacing the second chat_stream() SSE call with stream_text() — is correct, well-motivated, and well-tested. However the PR bundles five independent features beyond that fix, which makes it harder to review and harder to revert selectively if a problem appears.

Bundled features not mentioned in the PR description:

1. HTTP-based MCP server support (mcp.rs, config.rs)
2. Kimi native tool-call parsing (llm.rs)
3. /query-rewrite toggle command (telegram.rs, agent.rs, config.rs)
4. Entity-based Telegram markdown formatting (new utils/ files, telegram.rs)
5. Live tool-status streaming into the Telegram message during execution (agent.rs, tool_notifier.rs)

This is a common Copilot PR pattern — the agent stacks several related improvements in one branch. Each feature is individually reasonable, but the PR scope is much larger than the description suggests.

---

Issues

src/agent.rs — Redundant if/else in final response path

// src/agent.rs ~line 436–442
let final_content = if let Some(tx) = stream_token_tx {
    LlmClient::stream_text(content.clone(), tx).await.ok();
    content.clone()   // ← same as else branch
} else {
    content.clone()   // ← same as if branch
};

Both branches return content.clone(). The if/else is only needed for the side-effect of streaming. Fix this →

src/llm.rs — stream_text adds ~1.3s of artificial latency for long responses

The 2ms inter-chunk delay with 30-char chunks means a 10,000-character response takes ~667 chunks × 2ms ≈ 1.3 seconds of pure artificial delay before the final message is complete. For Kimi's 11-minute runs this is irrelevant, but for short fast models it will feel sluggish. Consider making CHUNK_SIZE and the delay configurable, or only inserting delay every N chunks:

// current — 2ms every 30 chars
tokio::time::sleep(tokio::time::Duration::from_millis(2)).await;

// alternative — throttle at Telegram's edit rate (~20 edits/sec) instead
// e.g. yield every 150 chars (5 chunks), keeping 2ms total edit period

src/mcp.rs — Empty auth header sent when auth_token is None

// src/mcp.rs ~line 57
let transport_config = StreamableHttpClientTransportConfig::with_uri(url.to_string())
    .auth_header(config.auth_token.clone().unwrap_or_default());
//                                         ^^^^^^^^^^^^^^^^
// unwrap_or_default() produces "" — may send `Authorization: Bearer ` header

If rmcp's auth_header does not skip empty strings, every HTTP MCP request to auth-free servers will carry a malformed Authorization: Bearer header. Prefer a conditional: Fix this →

let mut transport_config = StreamableHttpClientTransportConfig::with_uri(url.to_string());
if let Some(token) = &config.auth_token {
    transport_config = transport_config.auth_header(token.clone());
}

src/utils/markdown_entities.rs — O(n²) UTF-16 counting

// Called on every markdown event — O(n) each time, O(n²) total
let start_utf16 = plain.encode_utf16().count();
plain.push_str(&text);
let end_utf16 = plain.encode_utf16().count();

encode_utf16().count() re-scans the entire plain string every time. For a 4 KB response with dozens of formatting spans this is negligible, but it's easy to fix with a running counter:

// Maintain a running UTF-16 length instead
let mut utf16_len: usize = 0;
// On each text push: utf16_len += text.encode_utf16().count();

Fix this →

src/utils/markdown_entities.rs — Paragraphs emit only one \n, not two

TagEnd::Paragraph => {
    plain.push('\n');  // standard prose expects \n\n between paragraphs
}

Telegram renders paragraphs as a single blank line when you have \n\n. The current single \n merges adjacent paragraphs visually.

src/utils/telegram_markdown.rs — Large block of dead code

The file exports only escape_text publicly and uses it in telegram.rs. The remaining functions — escape_code, find_unescaped, convert_inline, find_matching_bracket, convert_markdown_to_v2 (556 lines total) — are all #[allow(dead_code)] and unused. Per CLAUDE.md conventions, unused code should be deleted rather than kept with #[allow(dead_code)].

If convert_markdown_to_v2 is a planned fallback path, it should live in a separate PR with a clear use case; otherwise delete it.

src/platform/telegram.rs — Source-inspection tests are fragile

#[test]
fn test_final_flush_uses_entity_based_conversion() {
    let source = include_str!("telegram.rs");
    assert!(source.contains("markdown_to_entities"), ...);
}

This tests the presence of a string in the source file, not any observable behavior. It will pass even if the function is called with wrong arguments and break when the function is renamed. Replace with a behavioral test that exercises the final-flush code path with a known Markdown input and checks the resulting Telegram message entities.

src/platform/tool_notifier.rs — MCP server name parsing is fragile for multi-underscore names

// humanise_function_name strips only the FIRST verb prefix
let stripped = func
    .strip_prefix("query_")
    .or_else(|| func.strip_prefix("search_"))
    ...
    .unwrap_or(func);

And the server name extraction:

if let Some(sep) = rest.find('_') {
    let server = &rest[..sep];  // "google" from "google-workspace_func"
    let func = &rest[sep + 1..]; // "workspace_func" — server name ≠ "google-workspace"
}

For mcp_google-workspace_query_gmail_emails, rest = google-workspace_query_gmail_emails. find('_') finds the underscore at position 6 (google-) not the one after workspace. Wait — google-workspace uses a hyphen, not underscore, so find('_') finds the _ between workspace and query. Let me trace:

• rest = "google-workspace_query_gmail_emails"
• find('_') → position 16 (after google-workspace)
• server = "google-workspace" ✓
• func = "query_gmail_emails"
• After strip_prefix("query_") → "gmail_emails"
• Result: 📧 Gmail emails ✓

That path is actually fine. But for a tool like mcp_exa_search_web:

• rest = "exa_search_web"
• server = "exa" (not in the icon map → 🔧)
• func = "search_web" → stripped → "web" → Web
• Result: 🔧 Web — loses "search" context

Minor UX issue rather than a bug.

---

Non-blocking Observations

• stream_status_tx.try_send(status).ok() (agent.rs:381): try_send silently drops the status line if the channel is full. This is acceptable for UX status, but worth a debug! log on failure so it's visible during development.

• TagEnd::Item adds \n but list items in LLM output typically don't need extra padding — this is fine.

• Entity clamping across split boundaries (markdown_entities.rs:277–289): An entity that spans a chunk split point is clamped to each side. The clamped halves will show partial bold/italic in each chunk, which is visually acceptable but unusual. No test covers this edge case.

• config.example.toml: The new query_rewriter_enabled and HTTP MCP examples are clear and well-documented.

• src/config.rs tests: Thorough and cover all new fields.

---

What Works Well

• The core fix (stream_text) is clean and correct — eliminates the root cause with minimal surface area change.
• Kimi tool-call parsing is well-isolated with good test coverage.
• The entity-based Telegram formatting approach is architecturally superior to MarkdownV2 escaping and the UTF-16 offset handling with the precomputed boundary table (split_entities) is correct.
• HTTP MCP server support is cleanly separated into connect_http / connect_stdio / register_client.
• Test coverage across new functionality is strong (45+ new tests).

[Copilot]

Pull request overview

This PR aims to prevent silently truncated Telegram replies by removing the post-hoc “replay via second LLM streaming call” approach and instead streaming the already-complete final content locally. It also adds several related platform/config enhancements (Telegram entity formatting, MCP HTTP transport support, query rewrite toggles, tool-name humanisation, and Kimi tool-call parsing).

Changes:

• Replace the “second LLM SSE streaming call” with local streaming of the already-complete response text (LlmClient::stream_text), avoiding truncation on dropped streams.
• Switch Telegram final message formatting to an entity-based pipeline (markdown_to_entities + split_entities) while keeping intermediate edits as plain text.
• Extend MCP configuration/connection to support Streamable HTTP servers, add query rewrite toggle support, and improve tool status/name formatting.

Reviewed changes

Copilot reviewed 12 out of 14 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/agent.rs	Streams final content via stream_text; adds query rewrite enablement logic; injects tool status into token stream.
src/llm.rs	Adds stream_text; removes SSE streaming; adds Kimi tool-call text parsing/fix-up.
src/platform/telegram.rs	Uses MarkdownV2 escaping for command replies; final flush now uses entity-based formatting + chunking.
src/utils/markdown_entities.rs	New Markdown→(plain text, Telegram entities) converter + splitter for Telegram length limits.
src/utils/telegram_markdown.rs	New MarkdownV2 escaping/conversion helpers (used for command responses via escape_text).
src/utils/mod.rs	Exposes the new utils modules.
src/platform/tool_notifier.rs	Adds friendly tool-name mapping, tool status line builder, and updates args preview formatting/tests.
src/mcp.rs	Adds HTTP transport path (Streamable HTTP) and refactors connect/register flow.
src/config.rs	Makes MCP command optional and adds url/auth_token; adds memory.query_rewriter_enabled.
src/bin/setup.rs	Allows parsing HTTP MCP fields in existing configs without breaking the setup wizard.
config.example.toml	Documents query rewriting and HTTP MCP server configuration examples.
README.md	Notes the new Markdown parsing dependency for Telegram entity formatting.
Cargo.toml	Adds pulldown-cmark and enables rmcp HTTP transport feature.
Cargo.lock	Locks new dependencies pulled in by pulldown-cmark and rmcp HTTP transport.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

friendly_tool_name parses MCP tool names by splitting on the first _ after mcp_, but MCP tool names are constructed as mcp_{server_name}_{tool_name} and server_name can itself contain underscores. In that case the server/icon detection will be wrong and the function name humanisation will be applied to part of the server name. Consider parsing by matching against the configured/connected server names, or splitting from the right (last _) if tool names cannot contain underscores, or storing server/tool metadata alongside the tool log.

[Copilot]

This repeatedly computes plain.encode_utf16().count() to get offsets/lengths. Because encode_utf16().count() scans the whole accumulated plain each time, the conversion can become O(n^2) on long LLM outputs. Consider tracking a running plain_utf16_len counter and updating it incrementally as you append text/newlines, so start/end offsets are O(1).

[Copilot]

The comment says “Add blank line after each paragraph”, but only a single \n is appended here, which collapses Markdown paragraphs separated by a blank line into a single newline in plain. If the intent is to preserve paragraph breaks, append \n\n (or otherwise preserve the original spacing) and adjust the trailing-newline trimming accordingly.

[Copilot]

split_entities assumes find_split_point always returns a UTF-16 offset strictly greater than chunk_utf16_start. If max_utf16_len is smaller than a single character’s UTF-16 length (e.g. 1 with an emoji/surrogate pair), find_split_point can return start_utf16, causing chunk_utf16_start to never advance and the loop to spin forever. Add a guard to ensure progress (e.g. if split_utf16 == chunk_utf16_start, advance to the next char boundary).

Suggested change

	let split_utf16 = find_split_point(
	text,
	&char_utf16_boundaries,
	chunk_utf16_start,
	chunk_utf16_end_ideal,
	);
	let mut split_utf16 = find_split_point(
	text,
	&char_utf16_boundaries,
	chunk_utf16_start,
	chunk_utf16_end_ideal,
	);
	// Ensure progress even if `find_split_point` cannot find a boundary inside
	// the requested window (for example, when `max_utf16_len` is smaller than
	// a single character's UTF-16 length, such as an emoji/surrogate pair).
	if split_utf16 <= chunk_utf16_start {
	split_utf16 = char_utf16_boundaries
	.iter()
	.map(|&(_, utf16)| utf16)
	.find(|&utf16| utf16 > chunk_utf16_start)
	.unwrap_or(total_utf16);
	}

[Copilot]

This PR’s title/description focuses on removing a second LLM streaming API call, but the changeset also introduces multiple additional features/behavior changes (e.g. MCP HTTP transport config fields, query rewriting toggle, Telegram entity-based formatting). Please either update the PR description to cover these scope expansions or split the unrelated changes into separate PRs to keep review/rollback risk manageable.

[github-actions]

Claude finished @chinkan's task in 2m 43s —— View job

---

Code Review

Todo List:

• Gather context and understand the PR
• Review all 14 changed files
• Post comprehensive review feedback

---

Summary

The core fix — replacing chat_stream() with stream_text() — is clean, correct, and well-motivated. The PR also bundles five additional features (HTTP MCP support, Kimi tool-call parsing, /query-rewrite toggle, entity-based Telegram formatting, live tool-status streaming) that are individually reasonable but significantly expand the review surface. Each feature has solid test coverage (45+ new tests).

---

Issues

src/agent.rs — Redundant if/else in final response path

Both branches of the streaming block return content.clone() — the if/else exists only for the side-effect:

// src/agent.rs ~line 90–95
let final_content = if let Some(tx) = stream_token_tx {
    LlmClient::stream_text(content.clone(), tx).await.ok();
    content.clone()   // ← identical to else
} else {
    content.clone()   // ← identical to if
};

Simplify to make intent explicit:

if let Some(tx) = stream_token_tx {
    LlmClient::stream_text(content.clone(), tx).await.ok();
}
let final_content = content;

Fix this →

---

src/mcp.rs — Empty Authorization header sent when auth_token is None

// src/mcp.rs ~line 57
let transport_config = StreamableHttpClientTransportConfig::with_uri(url.to_string())
    .auth_header(config.auth_token.clone().unwrap_or_default());
//                                         ^^^^^^^^^^^^^^^^
// Sends: Authorization: Bearer  (empty string)

unwrap_or_default() produces "". If rmcp doesn't skip empty auth headers, every HTTP MCP request to auth-free servers will carry a malformed Authorization: Bearer header. Use a conditional:

let mut transport_config = StreamableHttpClientTransportConfig::with_uri(url.to_string());
if let Some(token) = &config.auth_token {
    transport_config = transport_config.auth_header(token.clone());
}

Fix this →

---

src/utils/markdown_entities.rs — O(n²) UTF-16 counting

encode_utf16().count() rescans the entire accumulated plain string on every text event. For a 4 KB response with many formatting spans, this is quadratic:

// Called on every Event::Text and every formatting open/close — O(n) each time
let start_utf16 = plain.encode_utf16().count();  // O(n)
plain.push_str(&text);
let end_utf16 = plain.encode_utf16().count();    // O(n)

Maintain a running counter instead:

let mut utf16_len: usize = 0;
// When pushing text:
let start_utf16 = utf16_len;
plain.push_str(&text);
utf16_len += text.encode_utf16().count();  // O(pushed text only)
let end_utf16 = utf16_len;

Fix this →

---

src/utils/markdown_entities.rs — Paragraphs get single \n, not \n\n

TagEnd::Paragraph => {
    plain.push('\n');  // single newline merges adjacent paragraphs visually
}

Telegram renders blank lines between paragraphs with \n\n. The current single \n makes consecutive paragraphs run together. Change to plain.push_str("\n\n") (and adjust the trailing-newline trim accordingly).

Fix this →

---

src/utils/markdown_entities.rs — Infinite loop when max_utf16_len < surrogate-pair width

In split_entities, if find_split_point returns chunk_utf16_start (possible when max_utf16_len = 1 and the next char is a surrogate pair), chunk_utf16_start never advances:

while chunk_utf16_start < total_utf16 {
    let split_utf16 = find_split_point(...);
    // If split_utf16 == chunk_utf16_start → infinite loop
    chunk_utf16_start = split_utf16;
}

Add a progress guard after find_split_point:

// Guard: ensure we always advance at least one character
if split_utf16 <= chunk_utf16_start {
    split_utf16 = char_utf16_boundaries
        .iter()
        .map(|&(_, u)| u)
        .find(|&u| u > chunk_utf16_start)
        .unwrap_or(total_utf16);
}

While MAX_UTF16 = 4090 makes this unreachable in production, the guard prevents a panic/hang if the constant is ever lowered. Fix this →

---

src/utils/telegram_markdown.rs — 537 lines of dead code

escape_code, find_unescaped, convert_inline, find_matching_bracket, convert_markdown_to_v2 — all marked #[allow(dead_code)] and unused. Only escape_text is called externally. Per CLAUDE.md:

Avoid backwards-compatibility hacks... If you are certain that something is unused, you can delete it completely.

Delete the dead functions. If convert_markdown_to_v2 is a planned fallback, it should live in a separate PR with a clear use-case. Keeping 537 dead lines increases maintenance burden and confuses future contributors.

Fix this →

---

src/platform/telegram.rs — Source-inspection tests are fragile

#[test]
fn test_final_flush_uses_entity_based_conversion() {
    let source = include_str!("telegram.rs");
    assert!(source.contains("markdown_to_entities"), ...);
}

This tests the presence of a string in source code, not behavior. It will pass even if the function is called with wrong arguments and will break on a refactor. Replace with a behavioral test using a known Markdown input and checking the resulting output. These tests belong in the CI pipeline but don't actually validate correctness.

---

src/config.rs — MCP server with neither command nor url silently proceeds to startup

A misconfigured [[mcp_servers]] with no command and no url will pass config parsing and only fail when connect() is called. Consider adding a validation step:

impl McpServerConfig {
    pub fn validate(&self) -> Result<()> {
        if self.command.is_none() && self.url.is_none() {
            anyhow::bail!(
                "MCP server '{}': must have either 'command' (stdio) or 'url' (HTTP)",
                self.name
            );
        }
        Ok(())
    }
}

---

Minor Observations

• src/agent.rs:381 — stream_status_tx.try_send(status).ok(): silently drops the status line if the channel is full. A debug!("tool status channel full — dropping: {}", status) would help during development.

• src/llm.rs — stream_text artificial latency: 30-char chunks × 2ms = ~1.3 s for a 20 KB response. Negligible for 11-minute Kimi runs but noticeable for fast short-context models. Consider a configurable delay or skipping delay every N chunks.

• src/platform/tool_notifier.rs:49 — MCP server name parsing works correctly for hyphenated names like google-workspace (hyphen before the underscore separator). Minor UX gap: mcp_exa_search_web → 🔧 Web (strips "search" context). Not a bug, just worth noting.

• Entity clamping in split_entities: An entity spanning a chunk boundary produces partial bold/italic on each side. Visually acceptable; no test covers it. Could add a regression test.

---

What Works Well

• Core fix (stream_text): Eliminates the root cause cleanly. The new implementation is correct, testable, and has no network dependency.
• Kimi tool-call parsing: Well-isolated, thorough test coverage for edge cases (invalid JSON, multiple calls, index embedding).
• Entity-based Telegram formatting: Architecturally superior to MarkdownV2 escaping — no 400 errors, no escaping required. The UTF-16 boundary table in split_entities is correct.
• HTTP MCP support: Clean stdio/HTTP dispatch via connect(). Config is backward-compatible (existing stdio configs work without change).
• Test coverage: 45+ new tests across the new code paths is excellent for this codebase.

[chinkan]

@copilot apply changes based on the comments in this thread

[chinkan]

@claude[agent] fix all issue in your comments

[Copilot]

@chinkan Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub.

You can ask me to try again later by mentioning me in a new comment.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: d9da1fae-c410-4624-80ac-5fcec9327468

Sorry for the inconvenience!

[Copilot]

@chinkan Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub.

You can ask me to try again later by mentioning me in a new comment.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: d9da1fae-c410-4624-80ac-5fcec9327468

Sorry for the inconvenience!

[Copilot]

@chinkan Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub.

You can ask me to try again later by mentioning me in a new comment.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: d9da1fae-c410-4624-80ac-5fcec9327468

Sorry for the inconvenience!

[Copilot]

@chinkan Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub.

You can ask me to try again later by mentioning me in a new comment.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: d9da1fae-c410-4624-80ac-5fcec9327468

Sorry for the inconvenience!

[Copilot]

@chinkan Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub.

You can ask me to try again later by mentioning me in a new comment.

If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: d9da1fae-c410-4624-80ac-5fcec9327468

Sorry for the inconvenience!

[chinkan]

@copilot for google model, it has error when call tools:

2026-04-09T15:31:13.442829Z DEBUG rustfox::agent: Trying iteration 0: messages length: 2
2026-04-09T15:31:13.446542Z DEBUG rustfox::llm: Sending request to OpenRouter url=https://openrouter.ai/api/v1/chat/completions model=google/gemma-4-26b-a4b-it:free message_count=2 tool_count=136
2026-04-09T15:31:14.128454Z WARN rustfox::platform::telegram: Agent processing failed error=OpenRouter API error (400 Bad Request): {"error":{"message":"Provider returned error","code":400,"metadata":{"raw":"{\n "error": {\n "code": 400,\n "message": "* GenerateContentRequest.tools[0].function_declarations[111].parameters.required[0]: property is not defined\n* GenerateContentRequest.tools[0].function_declarations[111].parameters.required[1]: property is not defined\n",\n "status": "INVALID_ARGUMENT"\n }\n}\n","provider_name":"Google AI Studio","is_byok":false}},"user_id":"user_2jBN3mTLVzszaJfFX0pf5laQPh8"}

use superpower to create plan to research deeply on web and context 7 and fix above issue

[Copilot]

Pull request overview

Copilot reviewed 12 out of 14 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The comment says this trims “at most two” trailing newlines, but the while plain.ends_with('\n') loop will remove all trailing newlines. Either cap the trim to two newlines or update the comment to match the current behavior (also consider whether trimming beyond two can remove intentional block spacing at EOF).

Suggested change

	while plain.ends_with('\n') && plain_utf16_len > 0 {
	plain.pop();
	plain_utf16_len -= 1;
	for _ in 0..2 {
	if plain.ends_with('\n') && plain_utf16_len > 0 {
	plain.pop();
	plain_utf16_len -= 1;
	} else {
	break;
	}

[Copilot]

When verbose mode is enabled, tool progress is already shown via ToolCallNotifier (separate “⏳ Working…” message). This additional streaming of tool status lines into the main response token stream will duplicate the tool UI and permanently prepend status lines to the final assistant message. Consider choosing a single mechanism (either keep the notifier message, or stream status into the main message and disable the notifier for that mode).

Suggested change

	// Stream tool status into the Telegram message only when
	// tool-progress notifications are enabled, to avoid
	// prepending status lines to otherwise silent/final output.
	if tool_event_tx.is_some() {
	if let Some(ref tx) = stream_status_tx {
	let status =
	crate::platform::tool_notifier::format_tool_status_line(
	&tool_call.function.name,
	&args_preview,
	);
	tx.try_send(status).ok();
	}
	}

[Copilot]

stream_text collects the entire response into a Vec<char> before chunking, which duplicates memory and adds O(n) upfront cost for large responses. This can be chunked without allocating a char vector by iterating over char_indices() and slicing the original String on UTF-8 boundaries.

[Copilot]

The comment says known_servers is “Sorted by length descending to match longest first”, but the array isn’t actually sorted/enforced. Either sort it (e.g., build a vec and sort by name length) or drop/adjust the comment so future edits don’t rely on a property that isn’t guaranteed.

[Copilot]

@copilot for google model, it has error when call tools:

2026-04-09T15:31:13.442829Z DEBUG rustfox::agent: Trying iteration 0: messages length: 2
...

Fixed in commit 1e79161.

The root cause was Google AI Studio's strict JSON Schema validation: any field listed in required must also be defined in properties, and additionalProperties / $schema / $defs / $ref are not accepted.

Added sanitize_parameters() in src/llm.rs that runs before every API call and:

1. Removes required entries that reference undefined properties
2. Strips additionalProperties, $schema, $defs, $ref
3. Recurses into nested object properties and array items

This is applied transparently in chat_with_model() for all tools (both built-in and MCP-sourced), so any model — including Google Gemini — will receive clean schemas regardless of what the MCP server returned.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 14 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

src/platform/telegram.rs:310

• Seeding current_msg_id with the non-verbose placeholder interacts badly with the split branch: when buffer.len() > TELEGRAM_STREAM_SPLIT (likely before the first 500ms edit), the code sends a new message and overwrites current_msg_id, losing the placeholder MessageId. That can leave the original "⏳ Thinking..." message orphaned (never edited or deleted) and can also duplicate content across messages. Consider tracking the placeholder id separately and, on split, either (a) edit the current message to the chunk and then reset current_msg_id = None for the next chunk, or (b) delete the placeholder before sending the first real chunk, so the placeholder cannot remain visible.

        let mut buffer = String::new();
        // Seed current_msg_id with the placeholder so the first edit reuses it.
        let mut current_msg_id: Option<teloxide::types::MessageId> = placeholder_msg_id;
        let mut last_action = Instant::now();
        let mut rx = stream_token_rx;

        while let Some(token) = rx.recv().await {
            buffer.push_str(&token);

            // When buffer exceeds split threshold, send a NEW message and reset
            if buffer.len() > TELEGRAM_STREAM_SPLIT {
                match stream_bot.send_message(stream_chat_id, &buffer).await {
                    Ok(new_msg) => {
                        current_msg_id = Some(new_msg.id);
                        buffer.clear();
                    }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The doc comment says that if stripping nullable union members leaves only one anyOf/oneOf/allOf variant, the schema is "inlined (unwrapped)", but the implementation only removes the {"type":"null"} variants and never unwraps a single remaining variant. Either implement the unwrapping logic or adjust the comment to match the current behavior.

Suggested change

/// If stripping removes all but one variant, the schema is inlined (unwrapped).

[Copilot]

After removing {"type":"null"} variants from anyOf/oneOf/allOf, the array can become empty (e.g., if a schema was only [ {"type":"null"} ]). Leaving an empty union in the schema is typically invalid and may still trigger provider-side validation errors. Consider removing the key entirely when the variants array becomes empty (and possibly unwrapping when it becomes length 1).

[Copilot]

utf16_to_byte's doc comment says it returns the byte position of the first char whose UTF-16 start is >= utf16_off, but the Err(idx) branch returns boundaries[idx - 1].0 (the previous boundary, i.e. <= utf16_off). Either adjust the implementation to match the stated semantics or update the comment so future callers don’t rely on the wrong behavior.

[chinkan]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Applied all three review comments in 88e50e4:

1. src/llm.rs:82 — sanitize_parameters() doc comment now accurately describes the unwrapping/removal behavior.
2. src/llm.rs:135-153 — After stripping {"type":"null"} variants, empty union arrays are removed entirely, and single-variant unions are unwrapped (inlined into parent object). Tests updated to verify.
3. src/utils/markdown_entities.rs:326-338 — utf16_to_byte doc comment corrected to say it returns the byte position at or just before utf16_off (matching the Err(idx) branch behavior).