Fix agent name validation and enum string support #3979
Conversation
- Add validation to reject invalid Python identifiers (e.g., hyphens) - Import and use validate_app_name from apps.app module - Display clear error message when invalid names are provided - Add comprehensive tests for valid and invalid agent names Fixes google#3977
…h_type - Add field_validator to convert string enum names (EXACT, IN_ORDER, ANY_ORDER) to enum values - Maintain backward compatibility with integer and enum values - Provide clear error message for invalid string values - Add comprehensive tests covering all input types Fixes google#3711
Summary of ChangesHello @sreeharsha1902, 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 enhances the robustness and user experience of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![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 introduces two valuable fixes. The agent name validation in adk create will prevent confusing downstream errors by providing immediate, clear feedback to the user. The addition of string support for ToolTrajectoryCriterion.match_type improves usability and aligns the implementation with the documentation, while thoughtfully maintaining backward compatibility. The changes are well-implemented and accompanied by comprehensive tests. I've suggested a few minor refactorings in the tests to improve conciseness and in the error handling to enhance maintainability, but overall this is a solid contribution.
| raise ValueError( | ||
| f"Invalid match_type: '{v}'. Must be one of: EXACT, IN_ORDER," | ||
| " ANY_ORDER" | ||
| ) |
There was a problem hiding this comment.
To improve maintainability, you can dynamically generate the list of valid match_type values from the MatchType enum itself. This avoids having to manually update the error string if the enum members change in the future.
| raise ValueError( | |
| f"Invalid match_type: '{v}'. Must be one of: EXACT, IN_ORDER," | |
| " ANY_ORDER" | |
| ) | |
| raise ValueError( | |
| f"Invalid match_type: '{v}'. Must be one of: " | |
| f"{', '.join(cls.MatchType.__members__)}" | |
| ) |
| def test_run_cmd_rejects_invalid_agent_names( | ||
| monkeypatch: pytest.MonkeyPatch, tmp_path: Path | ||
| ) -> None: | ||
| """run_cmd should reject agent names with hyphens or other invalid chars.""" | ||
| monkeypatch.setattr(os, "getcwd", lambda: str(tmp_path)) | ||
|
|
||
| # Test hyphen rejection | ||
| with pytest.raises(click.UsageError) as exc_info: | ||
| cli_create.run_cmd( | ||
| "my-agent", | ||
| model="gemini-2.5-flash", | ||
| google_api_key="test-key", | ||
| google_cloud_project=None, | ||
| google_cloud_region=None, | ||
| type="code", | ||
| ) | ||
| assert "must be a valid identifier" in str(exc_info.value) | ||
|
|
||
| # Test other invalid characters | ||
| with pytest.raises(click.UsageError) as exc_info: | ||
| cli_create.run_cmd( | ||
| "my agent", # space | ||
| model="gemini-2.5-flash", | ||
| google_api_key="test-key", | ||
| google_cloud_project=None, | ||
| google_cloud_region=None, | ||
| type="code", | ||
| ) | ||
| assert "must be a valid identifier" in str(exc_info.value) | ||
|
|
||
| # Test reserved name | ||
| with pytest.raises(click.UsageError) as exc_info: | ||
| cli_create.run_cmd( | ||
| "user", | ||
| model="gemini-2.5-flash", | ||
| google_api_key="test-key", | ||
| google_cloud_project=None, | ||
| google_cloud_region=None, | ||
| type="code", | ||
| ) | ||
| assert "reserved for end-user input" in str(exc_info.value) | ||
|
|
||
|
|
||
| def test_run_cmd_accepts_valid_agent_names( | ||
| monkeypatch: pytest.MonkeyPatch, tmp_path: Path | ||
| ) -> None: | ||
| """run_cmd should accept valid Python identifiers.""" | ||
| monkeypatch.setattr(os, "getcwd", lambda: str(tmp_path)) | ||
| monkeypatch.setattr(click, "prompt", lambda *a, **k: "1") # Choose type | ||
|
|
||
| # Valid names should work | ||
| cli_create.run_cmd( | ||
| "my_agent", | ||
| model="gemini-2.5-flash", | ||
| google_api_key="test-key", | ||
| google_cloud_project=None, | ||
| google_cloud_region=None, | ||
| type="code", | ||
| ) | ||
| assert (tmp_path / "my_agent").exists() | ||
|
|
||
| cli_create.run_cmd( | ||
| "agent123", | ||
| model="gemini-2.5-flash", | ||
| google_api_key="test-key", | ||
| google_cloud_project=None, | ||
| google_cloud_region=None, | ||
| type="code", | ||
| ) | ||
| assert (tmp_path / "agent123").exists() |
There was a problem hiding this comment.
These two test functions, test_run_cmd_rejects_invalid_agent_names and test_run_cmd_accepts_valid_agent_names, contain repeated logic for calling cli_create.run_cmd. You can make the tests more concise and easier to maintain by using pytest.mark.parametrize to test multiple inputs with a single test function for each case (valid and invalid).
@pytest.mark.parametrize(
"invalid_name, error_substring",
[
("my-agent", "must be a valid identifier"),
("my agent", "must be a valid identifier"),
("user", "reserved for end-user input"),
],
)
def test_run_cmd_rejects_invalid_agent_names(
monkeypatch: pytest.MonkeyPatch,
tmp_path: Path,
invalid_name: str,
error_substring: str,
) -> None:
"""run_cmd should reject invalid agent names."""
monkeypatch.setattr(os, "getcwd", lambda: str(tmp_path))
with pytest.raises(click.UsageError) as exc_info:
cli_create.run_cmd(
invalid_name,
model="gemini-2.5-flash",
google_api_key="test-key",
google_cloud_project=None,
google_cloud_region=None,
type="code",
)
assert error_substring in str(exc_info.value)
@pytest.mark.parametrize("valid_name", ["my_agent", "agent123"])
def test_run_cmd_accepts_valid_agent_names(
monkeypatch: pytest.MonkeyPatch, tmp_path: Path, valid_name: str
) -> None:
"""run_cmd should accept valid Python identifiers."""
monkeypatch.setattr(os, "getcwd", lambda: str(tmp_path))
monkeypatch.setattr(click, "prompt", lambda *a, **k: "1") # Choose type
cli_create.run_cmd(
valid_name,
model="gemini-2.5-flash",
google_api_key="test-key",
google_cloud_project=None,
google_cloud_region=None,
type="code",
)
assert (tmp_path / valid_name).exists()| def test_tool_trajectory_criterion_accepts_string_match_type(): | ||
| """Tests that ToolTrajectoryCriterion accepts string enum values.""" | ||
| # Test string values (as documented) | ||
| criterion_exact = ToolTrajectoryCriterion(threshold=0.5, match_type="EXACT") | ||
| assert criterion_exact.match_type == ToolTrajectoryCriterion.MatchType.EXACT | ||
|
|
||
| criterion_in_order = ToolTrajectoryCriterion( | ||
| threshold=0.5, match_type="IN_ORDER" | ||
| ) | ||
| assert ( | ||
| criterion_in_order.match_type | ||
| == ToolTrajectoryCriterion.MatchType.IN_ORDER | ||
| ) | ||
|
|
||
| criterion_any_order = ToolTrajectoryCriterion( | ||
| threshold=0.5, match_type="ANY_ORDER" | ||
| ) | ||
| assert ( | ||
| criterion_any_order.match_type | ||
| == ToolTrajectoryCriterion.MatchType.ANY_ORDER | ||
| ) | ||
|
|
||
|
|
||
| def test_tool_trajectory_criterion_accepts_enum_match_type(): | ||
| """Tests that ToolTrajectoryCriterion still accepts enum values.""" | ||
| # Test enum values (for backward compatibility) | ||
| criterion = ToolTrajectoryCriterion( | ||
| threshold=0.5, match_type=ToolTrajectoryCriterion.MatchType.IN_ORDER | ||
| ) | ||
| assert criterion.match_type == ToolTrajectoryCriterion.MatchType.IN_ORDER | ||
|
|
||
|
|
||
| def test_tool_trajectory_criterion_accepts_int_match_type(): | ||
| """Tests that ToolTrajectoryCriterion still accepts integer values.""" | ||
| # Test integer values (0, 1, 2) | ||
| criterion_0 = ToolTrajectoryCriterion(threshold=0.5, match_type=0) | ||
| assert criterion_0.match_type == ToolTrajectoryCriterion.MatchType.EXACT | ||
|
|
||
| criterion_1 = ToolTrajectoryCriterion(threshold=0.5, match_type=1) | ||
| assert criterion_1.match_type == ToolTrajectoryCriterion.MatchType.IN_ORDER | ||
|
|
||
| criterion_2 = ToolTrajectoryCriterion(threshold=0.5, match_type=2) | ||
| assert criterion_2.match_type == ToolTrajectoryCriterion.MatchType.ANY_ORDER |
There was a problem hiding this comment.
The tests for valid match_type inputs (string, enum, int) are spread across three separate functions. You can consolidate these into a single, more concise test function using pytest.mark.parametrize. This will reduce code duplication and make it easier to see all valid input types at a glance.
@pytest.mark.parametrize(
"match_type_input, expected_enum",
[
# String values
("EXACT", ToolTrajectoryCriterion.MatchType.EXACT),
("IN_ORDER", ToolTrajectoryCriterion.MatchType.IN_ORDER),
("ANY_ORDER", ToolTrajectoryCriterion.MatchType.ANY_ORDER),
# Enum values
(ToolTrajectoryCriterion.MatchType.IN_ORDER, ToolTrajectoryCriterion.MatchType.IN_ORDER),
# Integer values
(0, ToolTrajectoryCriterion.MatchType.EXACT),
(1, ToolTrajectoryCriterion.MatchType.IN_ORDER),
(2, ToolTrajectoryCriterion.MatchType.ANY_ORDER),
],
)
def test_tool_trajectory_criterion_accepts_valid_match_types(
match_type_input, expected_enum
):
"""Tests that ToolTrajectoryCriterion accepts string, enum, and int values."""
criterion = ToolTrajectoryCriterion(threshold=0.5, match_type=match_type_input)
assert criterion.match_type == expected_enum
- Use dynamic enum member list in error messages for better maintainability - Consolidate test functions using pytest.mark.parametrize - Reduce code duplication and improve test readability Co-authored-by: gemini-code-assist[bot] <gemini-code-assist[bot]@users.noreply.github.com>
ryanaiagent
added
core
|
Hi @sreeharsha1902 , Thank you for your contribution! We appreciate you taking the time to submit this pull request. Your PR has been received by the team and is currently under review. We will provide feedback as soon as we have an update to share. |
|
Hi @wyf7107 , can you please review this. LGTM |
ryanaiagent
added
the
needs-review
3 participants
This PR addresses two separate issues:
Fix #3977: Validate agent names in 'adk create' command
The
adk createcommand now validates agent names before creating directories, rejecting invalid Python identifiers (like names with hyphens). This prevents the confusing error that occurs later when trying to load the agent.Changes:
validate_app_namefromapps.appmoduleclick.UsageErrorfor clear CLI feedbackExample:
$ adk create my-agent Error: Invalid app name 'my-agent': must be a valid identifier consisting of letters, digits, and underscores.Fix #3711: Accept string enum values for ToolTrajectoryCriterion.match_type
The
ToolTrajectoryCriterionclass now accepts string enum values ("EXACT", "IN_ORDER", "ANY_ORDER") as documented, in addition to integer and enum values for backward compatibility.Changes:
@field_validatorto convert string enum names to enum valuesExample:
Testing