fix: built-in time tool call failure

[spiritj]

Using qwen3.5-plus as llm,created a skill (will use built-in time tool)as below

Daily Quote Skill

强制要求

在输出任何内容之前，你必须先调用 time 工具获取当前日期。

调用参数只传 operation 字段，值为 "now"：

{ "operation": "now" }

在收到 time 工具的返回结果之前，不得生成任何输出。禁止使用估算日期作为主路径。

回退（仅限 time 工具返回错误时）

若 time 工具调用失败（报错或返回错误信息），不要重试，也不要报错给用户。
改用模型自身知识估算当前日期，并在日期后标注**（日期估算）**。

输出格式

but got error:
今日格言
› ○ Processing...
2026-03-09T10:00:48.086232Z INFO Skill activated skill_name="daily-quote" skill_version="0.2.0" trust=trusted trust_label="TRUSTED"
2026-03-09T10:00:48.087029Z INFO Tool attenuation applied min_trust=trusted tools_available=38 tools_removed=0 removed=[] explanation=All active skills are trusted (full trust), all tools available
○ Calling LLM...
○ Executing 1 tool(s)...
○ time
✗ time (failed)
2026-03-09T10:00:52.215443Z INFO Tool attenuation applied min_trust=trusted tools_available=38 tools_removed=0 removed=[] explanation=All active skills are trusted (full trust), all tools available
○ Calling LLM...

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves an issue where certain Large Language Models (LLMs), such as Qwen, would incorrectly provide empty strings for optional tool parameters instead of null, leading to tool call failures. The changes introduce a robust mechanism to normalize these parameters by converting empty strings to null before tool validation and execution. This ensures that tools correctly process optional inputs, improving compatibility and reliability across different LLM providers. Additionally, the user interface now provides more informative error messages for failed tool executions.

Highlights

• Tool Parameter Normalization: Introduced a new function normalize_tool_params to convert empty string values in tool parameters to null. This addresses an issue where some LLMs (e.g., Qwen) return empty strings for optional nullable parameters instead of null, causing tool execution failures.
• Enhanced Tool Execution Robustness: The execute_chat_tool_standalone function now applies the normalize_tool_params function before validating and executing tools, ensuring that tools correctly interpret optional parameters.
• Improved Tool Validation Logic: Modified the validator.rs to treat empty strings as valid for optional parameters during tool validation, preventing spurious errors when LLMs provide empty strings instead of null.
• Detailed Tool Failure Reporting: Updated the REPL channel to display more specific error messages when a tool call fails, providing better debugging information.
• Comprehensive Testing: Added new unit and integration tests to verify the correct behavior of tool parameter normalization, validation, and the time tool's handling of optional parameters.

Changelog

• .gitignore
  • Added entries to ignore macOS specific files (.DS_Store) and Cursor IDE configuration files (.cursor/).
• src/agent/dispatcher.rs
  • Implemented normalize_tool_params function to convert empty strings to null in serde_json::Value objects.
  • Integrated normalize_tool_params into execute_chat_tool_standalone to preprocess tool parameters.
  • Changed tool execution error logging from debug to warn level.
  • Added unit tests for normalize_tool_params covering various scenarios including nested objects and non-string values.
• src/channels/repl.rs
  • Modified StatusUpdate::ToolCompleted handling to display detailed error messages for failed tool calls in the REPL output.
• src/safety/validator.rs
  • Updated validate_recursive to skip validation for empty strings, treating them as absent optional parameters.
  • Added unit tests (test_validate_tool_params_empty_string_is_valid, test_validate_tool_params_non_empty_strings_still_validated) to confirm the new validation behavior.
• src/tools/builtin/time.rs
  • Added integration tests (test_now_with_empty_string_optional_params, test_now_missing_optional_params_succeeds) to ensure the time tool gracefully handles null or missing optional parameters.

Activity

• The author identified a bug where the time tool failed when called by qwen3.5-plus due to empty strings being passed for optional parameters instead of null.
• The PR provides a fix to address this specific tool call failure scenario.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request primarily addresses an issue where some LLMs return empty strings instead of null for optional tool parameters, causing validation and parsing failures. It introduces a normalize_tool_params function to convert empty strings to null before tool execution and modifies the parameter validator to treat empty strings as absent. Additionally, the REPL channel now displays detailed error messages for failed tool executions, tool errors are logged as warnings, and .gitignore is updated to include .DS_Store and .cursor/. Comprehensive tests were added to ensure the correct handling of empty and null optional parameters.

[zmanian]

Review: fix: built-in time tool call failure

The root cause analysis is correct: strict-mode LLM providers (Qwen/DashScope) emit "" instead of null for optional nullable parameters, which causes parse_timezone("") to fail. The fix approach of normalizing empty strings to null before validation/execution is reasonable. However, there are several issues that should be addressed before merging.

Issues to fix

1. Normalization is not applied in worker.rs and scheduler.rs (bug)

normalize_tool_params is only called in execute_chat_tool_standalone (dispatcher.rs), but there are two other independent tool execution paths that validate and execute tool params without going through that function:

• src/agent/worker.rs ~line 803: validates and executes tool params for background scheduler jobs
• src/agent/scheduler.rs ~line 488: validates and executes tool params for subtasks

If a Qwen-backed job runs via the scheduler or worker path, it will still hit the same empty-string failure. The normalization should either be applied in all three paths, or better yet, moved into validate_tool_params itself so it's impossible to miss.

2. Validator empty-string skip is a security concern

The change in src/safety/validator.rs makes the validator silently skip all empty strings:

if s.is_empty() {
    return;
}

This means empty strings bypass ALL validation checks (length, encoding, forbidden patterns, injection detection). While empty strings are benign, this is defense-in-depth code -- silently skipping values in the safety layer is the wrong place to handle an LLM provider quirk. The normalization in the dispatcher (converting "" to null) already solves the problem before validation runs. This validator change is redundant and weakens the safety layer. Remove it.

3. tracing::debug -> tracing::warn is unrelated and noisy

The change from debug! to warn! for tool call failures (line ~928 in the diff) is unrelated to the fix and will be noisy in production. Tool failures are expected (e.g., user provides bad input, network timeouts). debug is the correct level for operational tool failures. Leave this as debug!.

Suggestions (non-blocking)

4. Array empty-string normalization

normalize_tool_params recursively normalizes arrays, converting empty strings inside arrays to null too. This seems overly aggressive -- empty strings in arrays are legitimate values (e.g., a list of tags where "" might be meaningful). Consider only normalizing empty strings in object values (top-level and nested objects), not array elements.

5. .gitignore changes are unrelated

The .DS_Store and .cursor/ gitignore additions are fine on their own but should be in a separate commit/PR to keep the fix focused.

6. REPL error detail change is unrelated

The repl.rs change to show error details in the REPL output is a nice improvement but is unrelated to the time tool fix. Consider splitting it out.

What looks good

• The normalize_tool_params function itself is well-implemented and the recursive handling of nested objects is correct
• Good regression tests covering empty strings, nested objects, and non-string values
• The time tool tests for null optional params are useful
• The PR description clearly explains the reproduction scenario

Summary

The core fix (normalize empty strings to null before tool execution) is correct, but it needs to be applied consistently across all execution paths, and the validator safety bypass should be removed. The PR also mixes in several unrelated changes that should be split out.

[zmanian]

Re-review after merge commit 8c3678d (Merge branch 'staging' into main).

The merge commit brings the branch up to date with staging but does not address any of the three blocking issues from the previous review. The diff for the fix itself is unchanged.

Still outstanding (blocking):

1. Normalization missing in worker.rs and scheduler.rs -- normalize_tool_params is still only called in execute_chat_tool_standalone. The worker and scheduler tool execution paths remain vulnerable to the same empty-string failure with Qwen-backed jobs. Either add normalization to those paths or move it into validate_tool_params.

2. Validator empty-string bypass weakens safety layer -- The if s.is_empty() { return; } in src/safety/validator.rs still skips all validation for empty strings. This is redundant with the dispatcher normalization and creates an unnecessary hole in defense-in-depth. Remove it.

3. debug -> warn log level change is unrelated and noisy -- Tool failures logged at warn level will generate noise in production. Revert to debug.

Still outstanding (non-blocking, from previous review):

4. Array empty-string normalization is overly aggressive (empty strings in arrays may be intentional).
5. .gitignore changes are unrelated -- split into a separate commit.
6. REPL error detail change is unrelated -- split into a separate commit.

Please address at least items 1-3 before this can be approved.

[ilblackdragon]

Thanks for contribution. The time zone was already addressed but general issue with "" <> null is addressed in #1397