Skip to content

Fix: repair the eval-set route NameError, and select ruff F821 so undefined names cannot return to src/ - #200

Open
AmaadMartin wants to merge 3 commits into
mainfrom
fix/f821-undefined-names-src-lint-scope
Open

Fix: repair the eval-set route NameError, and select ruff F821 so undefined names cannot return to src/#200
AmaadMartin wants to merge 3 commits into
mainfrom
fix/f821-undefined-names-src-lint-scope

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    N/A
  2. Or, if no issue exists, describe the change:
    Problem: Ruff lint.select does not contain F821, so nothing in the repository ever checks for undefined names. Enabling the rule reports 8 findings in src/. One of them is a real defect: dev_server.py builds its request body from UserEvalSet, a name that has never existed here, so the deprecated route POST /dev/apps/{app_name}/eval_sets/{eval_set_id} raises NameError on every request. The other 7 are annotations that reference a name the module never imports, which breaks any consumer that resolves hints at runtime.

Solution: I triaged all 8 findings individually and fixed each at its root. UserEvalSet becomes EvalSet, the field's own declared type, already imported in dev_server.py. The other 7 get the import they were missing. Then F821 goes into lint.select so the class of defect cannot come back. I added no # noqa.

The Self findings were not breaking anything at runtime. environment_simulation_config.py has from __future__ import annotations, and pydantic does not resolve the return annotation of an after model validator. They are a typing defect, not a behaviour change.

Import placement follows the existing precedent in each file. api_server.py only needs the two eval-manager names for constructor annotations, so they go under TYPE_CHECKING, matching cli/utils/evals.py. The environment_simulation modules take plain module-level imports, matching all five siblings in that package. Self comes from typing_extensions because requires-python is >=3.10 and typing.Self is 3.11+; typing-extensions>=4.5,<5 is already a core dependency, so this adds none.

Scope. tests/ has 53 F821 findings and is deliberately out of scope; it is queued separately. The pre-commit ruff hook is scoped files: ^src/ and is the only ruff invocation in the repository, so enabling the rule cannot turn CI red on that backlog. I did not add a tests/** per-file-ignore, because that would relabel real undefined names as permanently allowed.

Collision check. I listed all 199 open pull requests on the fork and diffed every one that touches pyproject.toml, api_server.py, dev_server.py, or environment_simulation/. No PR does this change. #17 (fix/cli-mypy-strict) contains the same one-line UserEvalSet -> EvalSet fix inside a 32-file mypy pass, but it is CONFLICTING against main and it leaves the xfail(strict=True) marker in place, so it would turn the suite red. I branched from main rather than stacking on a conflicting branch, to keep this diff at 7 files.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

The repository already had the regression test for the one behavioural defect, at tests/unittests/cli/test_fast_api.py. It was marked @pytest.mark.xfail(strict=True, reason="legacy create-eval-set route references an undefined name"). Strict xfail reports an unexpected pass as a failure, so I removed the marker. I changed no assertion in the body. It still asserts response.status_code == 200 and that mock_eval_sets_manager.get_eval_set("test_app", "legacy_eval_set") is not None, and its fixture is a real InMemoryEvalSetsManager, not a mock.

I wrote no new test for the 7 annotation findings. They change no runtime behaviour, and the lint.select change is their regression guard.

Mutation proof. I ran each guard against the unfixed code and confirmed it fails.

  1. Reverted EvalSet back to UserEvalSet in dev_server.py:
    pytest tests/unittests/cli/test_fast_api.py -k test_create_eval_set_legacy -q
    -> 1 failed
    E     NameError: name 'UserEvalSet' is not defined
    src/google/adk/cli/dev_server.py:841: NameError
    
  2. Deleted the two new imports (BaseTool, Self) and ran the lint gate:
    ruff check src/ --output-format=concise
    -> Found 4 errors.
    .../environment_simulation_config.py:73:49: F821 Undefined name `Self`
    .../environment_simulation_config.py:112:41: F821 Undefined name `Self`
    .../strategies/base.py:33:13: F821 Undefined name `BaseTool`
    .../strategies/base.py:57:13: F821 Undefined name `BaseTool`
    

Both were restored, and the full run is green again.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

Two of the new imports are runtime imports, so I checked each touched module in a cold interpreter for a circular import. All succeed:

python -c "import google.adk.cli.api_server"
python -c "import google.adk.cli.dev_server"
python -c "import google.adk.tools.environment_simulation.strategies.base"
python -c "import google.adk.tools.environment_simulation.environment_simulation_engine"
python -c "import google.adk.tools.environment_simulation.environment_simulation_config"

Gates run locally on the pushed commit, with ruff==0.15.17, the pin in pyproject.toml and .pre-commit-config.yaml:

ruff check src/                     -> All checks passed!
pytest tests/unittests/cli/test_fast_api.py tests/unittests/tools/environment_simulation -q
                                    -> 138 passed, 5 skipped, 1 xfailed
isort --check-only --diff <6 files> -> exit 0
pyink --check --diff <6 files>      -> 6 files would be left unchanged

The remaining 1 xfailed is a different, pre-existing xfail that this change does not touch.

mypy. I ran the same A/B the CI gate runs (mypy . | grep "error:" | sed 's/:\([0-9]\+\):/::/g' | sort, then comm) against main:

  • new errors: 0
  • removed errors: 11 - the 8 name-defined errors matching the F821 findings, plus 3 no-any-return errors in dev_server.py that disappear because self.eval_sets_manager is no longer inferred as Any.

Total repository errors drop from 1600 to 1589.

CI. Unit Tests pass on Python 3.10, 3.11, 3.12, 3.13 and 3.14. Mypy Check passes on 3.10, 3.11, 3.12 and 3.13. A2A v0.3 Tests pass on all five versions.

Pre-commit Linter fails, for a reason unrelated to this change. The update-constraints hook regenerates the five constraints-*.txt files, finds a newer snapshot date, and exits 1. I reproduced it on main with an empty working tree:

git checkout main && pre-commit run update-constraints --all-files
-> update-constraints...Failed
-> exit code: 1

The same job fails the same way on #199, #198 and #197, which touch neither pyproject.toml nor the constraints files. #163, #56, #54 and #49 are already open against this hook. I did not fix it here.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 3 commits August 9, 2026 14:04
…ameError

The route POST /dev/apps/{app_name}/eval_sets/{eval_set_id} built its request
body from UserEvalSet, a name that never existed in this repository. Every
request to the route died with NameError before it reached the create path.
The field's own declared type is EvalSet, already imported in dev_server.py.

The regression test for this defect was marked xfail(strict=True). Strict xfail
reports an unexpected pass as a failure, so the marker is removed and the test
now guards the repaired route.
Each name was used in an annotation but never imported, so the annotation was
a static-typing lie. ADK resolves tool annotations at runtime with
get_type_hints(), which makes an unimportable name a real hazard.

api_server.py types two constructor parameters, so its imports go under
TYPE_CHECKING, matching cli/utils/evals.py. The environment_simulation modules
take plain module-level imports, matching every sibling in that package. Self
comes from typing_extensions because requires-python is >=3.10.
F821 was never selected, so nothing in the repository ran it. The pre-commit
ruff hook is scoped files: ^src/ and is the only ruff invocation, so this
enforces the rule on src/ without touching the tests/ backlog.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant