-
-
Notifications
You must be signed in to change notification settings - Fork 919
fix: cli mode google functions #450
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| import asyncio | ||||||||||||||||||||||||||||||||||||||
| import inspect | ||||||||||||||||||||||||||||||||||||||
| import json | ||||||||||||||||||||||||||||||||||||||
| import logging | ||||||||||||||||||||||||||||||||||||||
| import sys | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -24,6 +25,94 @@ | |||||||||||||||||||||||||||||||||||||
| logger = logging.getLogger(__name__) | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| def _is_fastapi_param_marker(default: Any) -> bool: | ||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||
| Check if a default value is a FastAPI parameter marker (Body, Query, etc.). | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| These markers are metadata for HTTP request parsing and should not be passed | ||||||||||||||||||||||||||||||||||||||
| directly to tool functions in CLI mode. | ||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||
| default_type = type(default) | ||||||||||||||||||||||||||||||||||||||
| return default_type.__module__ == "fastapi.params" and hasattr( | ||||||||||||||||||||||||||||||||||||||
| default, "get_default" | ||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+38
to
+41
|
||||||||||||||||||||||||||||||||||||||
| default_type = type(default) | |
| return default_type.__module__ == "fastapi.params" and hasattr( | |
| default, "get_default" | |
| ) | |
| # Use duck-typing instead of fragile module-name checks; FastAPI param | |
| # markers (Query, Body, etc.) expose both `in_` and `get_default`. | |
| return hasattr(default, "in_") and hasattr(default, "get_default") |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detecting Pydantic “undefined” via type(value).__name__ == "PydanticUndefinedType" is brittle across Pydantic versions/implementations. Prefer checking against known sentinels/types (e.g., importing the undefined sentinel/type if available) and/or handling inspect.Parameter.empty explicitly, to make required-field detection stable over dependency upgrades.
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detecting required defaults by comparing type(value).__name__ to PydanticUndefinedType is brittle across Pydantic versions (and can miss other sentinel defaults like pydantic.fields.Undefined / UndefinedType). Consider broadening this check (e.g., handling inspect.Parameter.empty as required, and supporting both Pydantic v1/v2 undefined sentinels) to keep CLI behavior stable across dependency upgrades.
| return value is Ellipsis or type(value).__name__ == "PydanticUndefinedType" | |
| # Treat standard Python/FastAPI markers as "required" | |
| if value is Ellipsis or value is inspect.Parameter.empty: | |
| return True | |
| # Handle Pydantic v1/v2 "undefined" sentinels without hard-coding imports. | |
| # - v1: pydantic.fields.Undefined (type: UndefinedType) | |
| # - v2: pydantic_core.PydanticUndefined (type: PydanticUndefinedType) | |
| t = type(value) | |
| type_name = getattr(t, "__name__", "") | |
| type_module = getattr(t, "__module__", "") | |
| if type_name in {"PydanticUndefinedType", "UndefinedType"} and type_module.startswith( | |
| "pydantic" | |
| ): | |
| return True | |
| return False |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If _extract_fastapi_default falls back to inspect.Parameter.empty, _is_required_marker_default will currently treat that as not-required, and _normalize_cli_args_for_tool may pass inspect.Parameter.empty into the tool function as a real argument value. Consider treating inspect.Parameter.empty as 'required/unresolved' (or raising) so CLI calls fail fast with a clear missing-arg error instead of passing a sentinel to user code.
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved_default can become inspect.Parameter.empty in _extract_fastapi_default() (line 59). Currently _is_required_marker_default() does not treat inspect.Parameter.empty as required, so _normalize_cli_args_for_tool() may inject inspect.Parameter.empty into normalized_args (line 95) and pass that sentinel into the tool, producing confusing downstream errors. Treat inspect.Parameter.empty as required (or avoid adding it to normalized_args) so the CLI raises a “missing required argument” error instead of passing the sentinel value through.
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The raised TypeError message says “required positional argument(s)”, but these tool calls are invoked via keyword arguments (fn(**call_args)), so the wording is misleading (and differs from Python’s typical “required keyword-only argument” phrasing when applicable). Consider emitting a message that matches Python’s conventions (e.g., “required keyword-only argument(s)” for keyword-only params, otherwise “required argument(s)”) or at least “required argument(s)” to avoid incorrect classification.
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The raised TypeError message says “required positional argument(s)”, but these tool calls are invoked via keyword arguments (fn(**call_args)), so the wording is misleading (and differs from Python’s typical “required keyword-only argument” phrasing when applicable). Consider emitting a message that matches Python’s conventions (e.g., “required keyword-only argument(s)” for keyword-only params, otherwise “required argument(s)”) or at least “required argument(s)” to avoid incorrect classification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
close_reasonclaims “+ regression tests”, but this PR diff doesn’t include any test changes. Either add the referenced regression tests in this PR or adjustclose_reasonto avoid documenting tests that weren’t actually implemented here.