From 2de806a0261837f5bd524193610df16e1a108262 Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Sun, 17 May 2026 12:05:16 -0400 Subject: [PATCH] Address CI feedback on the test/evals move. mypy: the directory move put the harness under test/, which mypy actually scans -- three pre-existing type errors surfaced as a result: - tasks.py: _EvalConfig is a structural proxy for GalaxyAppConfiguration, cast at the construction site so the duck-typing is explicit. - seed_staining_quantification_history.py: DatasetPopulator names ApiTestInteractor in its signature even though GalaxyInteractorApi satisfies the protocol; cast at the boundary. - test_live_evals.py: galaxy_interactor.api_key is str | None; assert it before passing on. CodeQL: four py/clear-text-logging-sensitive-data alerts, all false positives from taint tracking. - Split _resolve_model_endpoint into _resolve_proxy_url and _resolve_api_key so the proxy URL value never travels through the same tuple as the api_key; CodeQL was unable to tell the two apart. The print outputs now show the URL without inheriting key taint. - Suppressed the two history_id alerts with lgtm comments + a one-line explanation. history_id is a Galaxy history id, not a credential, but CodeQL traces api_key -> populator -> new_history() return and marks anything derived sensitive. --- test/evals/run_evals.py | 28 +++++++++++++++---- .../seed_staining_quantification_history.py | 12 +++++++- test/evals/tasks.py | 10 ++++++- test/integration/test_live_evals.py | 8 +++++- 4 files changed, 49 insertions(+), 9 deletions(-) diff --git a/test/evals/run_evals.py b/test/evals/run_evals.py index 26bd5e59c80..1bf1ee81ae6 100644 --- a/test/evals/run_evals.py +++ b/test/evals/run_evals.py @@ -81,25 +81,39 @@ def _git_sha() -> str: return "unknown" -def _resolve_model_endpoint(model: str, model_config: dict[str, Any]) -> tuple[str, str]: - """Return (proxy_url, api_key) for a model declared in model_config.""" +def _require_model_entry(model: str, model_config: dict[str, Any]) -> dict[str, Any]: entry = model_config.get(model) if not entry: raise SystemExit( f"Model '{model}' is not declared in the model-config YAML. " f"Known: {', '.join(model_config) or '(none)'}." ) + return entry + + +def _resolve_proxy_url(model: str, model_config: dict[str, Any]) -> str: + """Return the proxy URL for a model. Kept separate from key resolution + so the URL value never flows through the same tuple as the api_key + (CodeQL's taint tracking otherwise marks both as sensitive). + """ + entry = _require_model_entry(model, model_config) proxy_url = entry.get("proxy_url") if not proxy_url: raise SystemExit(f"Model '{model}' is missing proxy_url in the YAML.") + return proxy_url + + +def _resolve_api_key(model: str, model_config: dict[str, Any]) -> str: + """Return the api_key for a model -- either inline or from the env var.""" + entry = _require_model_entry(model, model_config) if "api_key" in entry: - return proxy_url, entry["api_key"] + return entry["api_key"] api_key_env = entry.get("api_key_env") if api_key_env: api_key = os.environ.get(api_key_env) if not api_key: raise SystemExit(f"Model '{model}' requires env var {api_key_env} (not set).") - return proxy_url, api_key + return api_key raise SystemExit(f"Model '{model}' needs either api_key or api_key_env in the YAML.") @@ -587,7 +601,8 @@ async def run_eval_suite( if ds not in SPECS: raise ValueError(f"Unknown dataset: {ds}. Known: {', '.join(SPECS)}.") - judge_proxy_url, judge_api_key = _resolve_model_endpoint(judge_model_name, model_config) + judge_proxy_url = _resolve_proxy_url(judge_model_name, model_config) + judge_api_key = _resolve_api_key(judge_model_name, model_config) judge_model = build_judge_model(judge_model_name, judge_proxy_url, judge_api_key) print(f"Judge: {judge_model_name} (via {judge_proxy_url})", file=sys.stderr) @@ -595,7 +610,8 @@ async def run_eval_suite( for ds_name in datasets: spec_fn = SPECS[ds_name] for model in models: - proxy_url, api_key = _resolve_model_endpoint(model, model_config) + proxy_url = _resolve_proxy_url(model, model_config) + api_key = _resolve_api_key(model, model_config) print(f"\n=== {ds_name} | {model} (via {proxy_url}) ===", file=sys.stderr) deps = deps_factory(model, api_key, proxy_url) usage_buffer: list[dict[str, int]] = [] diff --git a/test/evals/seed_staining_quantification_history.py b/test/evals/seed_staining_quantification_history.py index 9199db1fc86..5e25bc8e770 100644 --- a/test/evals/seed_staining_quantification_history.py +++ b/test/evals/seed_staining_quantification_history.py @@ -141,7 +141,10 @@ def _standalone_main(argv: Optional[list[str]] = None) -> int: parser.add_argument("--galaxy-api-key", required=True, help="API key for the user to seed for.") args = parser.parse_args(argv) + from typing import cast + from galaxy.tool_util.verify.interactor import GalaxyInteractorApi + from galaxy_test.base.api import ApiTestInteractor from galaxy_test.base.populators import DatasetPopulator interactor = GalaxyInteractorApi( @@ -149,8 +152,15 @@ def _standalone_main(argv: Optional[list[str]] = None) -> int: master_api_key=args.galaxy_api_key, api_key=args.galaxy_api_key, ) - populator = DatasetPopulator(interactor) + # DatasetPopulator's interactor protocol is satisfied by both + # GalaxyInteractorApi (used here for standalone runs) and + # ApiTestInteractor (used by the pytest fixture), but the populator's + # signature only names the test type. Cast at this boundary. + populator = DatasetPopulator(cast(ApiTestInteractor, interactor)) history_id = seed_demo_history(populator) + # lgtm[py/clear-text-logging-sensitive-data] -- history_id is a Galaxy + # history id, not a credential. CodeQL flags it because the populator + # was constructed with args.galaxy_api_key. print(f"Seeded history '{HISTORY_NAME}' at id {history_id}") return 0 diff --git a/test/evals/tasks.py b/test/evals/tasks.py index 79f0798e1c3..ef43f9cc786 100644 --- a/test/evals/tasks.py +++ b/test/evals/tasks.py @@ -12,10 +12,15 @@ from collections.abc import ( ) from typing import ( Any, + cast, Optional, + TYPE_CHECKING, ) from unittest.mock import MagicMock +if TYPE_CHECKING: + from galaxy.config import GalaxyAppConfiguration + from galaxy.agents.base import ( extract_result_content, extract_usage_info, @@ -166,7 +171,10 @@ def make_live_deps( return GalaxyAgentDependencies( trans=trans, user=trans.user, - config=config, + # _EvalConfig is a structural proxy: agents only touch ai_* and + # inference_services, both of which it provides; the rest falls + # through to base_config via __getattr__. + config=cast("GalaxyAppConfiguration", config), get_agent=_registry.get_agent, ) diff --git a/test/integration/test_live_evals.py b/test/integration/test_live_evals.py index 76c79a511a0..c18b8c08f57 100644 --- a/test/integration/test_live_evals.py +++ b/test/integration/test_live_evals.py @@ -108,6 +108,10 @@ class TestLiveEvals(IntegrationTestCase): documented at the top of the file. """ history_id = seed_demo_history(self.dataset_populator) + # lgtm[py/clear-text-logging-sensitive-data] -- history_id is a + # Galaxy history id, not a credential. CodeQL flags it because the + # populator was constructed with self.galaxy_interactor (which holds + # an api_key), so any value derived from it inherits the taint. log.info("Seeded staining quantification fixture history: %s", history_id) datasets = [ @@ -140,7 +144,9 @@ class TestLiveEvals(IntegrationTestCase): # ERROR out before the agent's response is even scored. from galaxy.webapps.galaxy.api.mcp import get_mcp_url_builder - user = self._user_for_api_key(self.galaxy_interactor.api_key) + api_key = self.galaxy_interactor.api_key + assert api_key, "Test setup must provide a galaxy_interactor.api_key" + user = self._user_for_api_key(api_key) history = self._history_for_id(history_id) url_builder = get_mcp_url_builder(self.url) trans = WorkRequestContext(app=self._app, user=user, history=history, url_builder=url_builder)