fix(skill): treat empty url param as absent when installing skills#1128
Conversation
github-actions
bot
added
scope: tool/builtin
Summary of ChangesHello, I'm Gemini Code Assist1! 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 Large Language Models (LLMs) sometimes provide empty strings for optional parameters instead of omitting them entirely. Specifically, it addresses the Highlights
Changelog
Using Gemini Code AssistThe 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
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 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
|
github-actions
bot
added
risk: medium
LLMs sometimes pass "" for optional parameters instead of omitting them. Previously, passing url: "" to skill_install would match the explicit-URL branch and attempt to fetch from an empty string, producing an invalid URL error instead of falling back to the catalog lookup. Fix by adding .filter(|s| !s.is_empty()) so an empty url is treated the same as a missing field. A unit test verifies the parameter filtering behaviour directly; the full execute path (catalog lookup + install) requires a real catalog and database and cannot be covered at the unit level.
micsama
force-pushed
the
fix/skill-install-empty-url
branch
from
26eb121 to
c8b27c0
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where an empty string for the url parameter in skill_install was causing an error. The fix, which treats an empty string as an absent parameter, is appropriate and is accompanied by a new unit test that verifies the behavior. My review includes a suggestion to centralize this parameter handling logic to improve maintainability, aligning with repository guidelines for refactoring duplicated code, which also aligns with the considerations you've mentioned in the pull request description.
| } else if let Some(url) = params | ||
| .get("url") | ||
| .and_then(|v| v.as_str()) | ||
| .filter(|s| !s.is_empty()) | ||
| { |
There was a problem hiding this comment.
This logic to handle optional non-empty string parameters is also duplicated in the new test. As you mentioned in the PR description, this issue of LLMs passing "" for optional parameters is a recurring pattern.
To improve maintainability and avoid code duplication, consider extracting this logic into a helper function. A centralized helper in src/tools/tool.rs would be ideal, for example:
// in src/tools/tool.rs
pub fn optional_non_empty_str<'a>(params: &'a serde_json::Value, name: &str) -> Option<&'a str> {
params
.get(name)
.and_then(|v| v.as_str())
.filter(|s| !s.is_empty())
}Using this helper would make the call site here cleaner and ensure consistency across different tools that face the same issue. Since modifying src/tools/tool.rs might be out of scope for this PR, this could be addressed in a follow-up. This suggestion aligns with the repository's guideline to prefer refactoring duplicated code into shared functions for better maintainability.
References
- When an issue is found in duplicated code, prefer refactoring into a shared function over applying localized fixes.
There was a problem hiding this comment.
That's a great point — the pattern is definitely worth centralizing, especially given that it now appears in at least three tools. I intentionally kept this PR narrow to stay low-risk and easy to review, but I agree a shared helper would be the right long-term home for this logic. I'll track it as a follow-up. That said, it might be worth waiting to see how the broader discussion in #1126 lands first — if the project ends up preferring a dispatcher-level normalization (as in #755), a per-tool helper may not be needed at all.
…earai#1128) LLMs sometimes pass "" for optional parameters instead of omitting them. Previously, passing url: "" to skill_install would match the explicit-URL branch and attempt to fetch from an empty string, producing an invalid URL error instead of falling back to the catalog lookup. Fix by adding .filter(|s| !s.is_empty()) so an empty url is treated the same as a missing field. A unit test verifies the parameter filtering behaviour directly; the full execute path (catalog lookup + install) requires a real catalog and database and cannot be covered at the unit level.
3 participants
Problem
LLMs sometimes pass
""for optional parameters instead of omitting them. Passingurl: ""toskill_installcaused the explicit-URL branch to match, attempting to fetch from an empty string and failing with an invalid URL error — instead of falling through to the catalog lookup as intended.Fix
Add
.filter(|s| !s.is_empty())to theurlparameter extraction so an empty string is treated the same as a missing field.Tests
A unit test verifies the parameter filtering behaviour directly. The full execute path (catalog lookup + install) requires a real catalog and database and cannot be covered at the unit level.
Related
This is the same class of issue described in #1126 — LLMs expressing "optional param not provided" as
""rather than omitting the field. That issue coverstime/routine; this PR addresses the same pattern inskill_install.PR #1127 fixes the
timetool variant. Open to feedback on whether a more centralised normalisation layer (as explored in #755) would be preferred over per-tool fixes.