fix: harden agent, benchmark, and UI against security, correctness, and stability issues

.github/scripts/round_manager.py

@@ -351,7 +351,12 @@ def evaluate_pr(pr: dict[str, Any], baseline_score: float, benchmark_config: str
             baseline_commit = os.environ.get("IMAGENT_BASELINE_COMMIT", "").strip()
             if baseline_commit:
                 command.extend(["--baseline-commit", baseline_commit])
-            completed = subprocess.run([str(part) for part in command], text=True, cwd=candidate_dir)
+            completed = subprocess.run(
+                [str(part) for part in command],
+                text=True,
+                cwd=candidate_dir,
+                env=_candidate_subprocess_env(os.environ),
+            )
         finally:
             subprocess.run(["git", "worktree", "remove", "--force", str(candidate_dir)], check=False)
 
@@ -366,6 +371,20 @@ def evaluate_pr(pr: dict[str, Any], baseline_score: float, benchmark_config: str
     return CandidateResult(pr=pr, report=report, score=score, delta=delta, head_sha=head_sha)
 
 
+def _candidate_subprocess_env(base_env: dict) -> dict:
+    """Return a copy of base_env safe to expose to untrusted candidate code.
+
+    The candidate benchmark subprocess executes contributor-supplied
+    ``agent/agent.py``, so secrets that only the orchestrator needs must not be
+    inherited. Strip the GitHub tokens to avoid exfiltration, but keep
+    ``OPENROUTER_API_KEY`` because the candidate needs it to generate images.
+    """
+    env = dict(base_env)
+    for name in ("GITHUB_TOKEN", "GH_TOKEN"):
+        env.pop(name, None)
+    return env
+
+
 def candidate_file_failures(filenames: list[str]) -> list[str]:
     failures: list[str] = []
     if CANDIDATE_AGENT_PATH not in filenames:
@@ -402,6 +421,12 @@ def policy_reasons(report: dict[str, Any]) -> list[str]:
 
 
 def is_merge_improvement_reason(reason: str) -> bool:
+    # NOTE: This is intentionally coupled to imagent-bench's human-readable policy
+    # reason strings (e.g. "score improvement 0.50 is below required 1.00"). A
+    # structured status field emitted by the benchmark would be more robust than
+    # matching English text; until that exists, guard against non-string input.
+    if not isinstance(reason, str):
+        return False
     return reason.startswith("score improvement ") and " is below required " in reason
 
 

agent/agent.py

@@ -6,6 +6,7 @@
 import os
 import re
 import urllib.error
+import urllib.parse
 import urllib.request
 from pathlib import Path
 from typing import Any
@@ -121,7 +122,7 @@ def _build_context(self, case: dict[str, Any]) -> dict[str, Any]:
         )
         sections = self._sections_from_prompt(prompt) or assets.get("sections", [])
         required_text = [title, *sections, *assets.get("required_text", [])]
-        if "reason" in case.get("allowed_tools", []):
+        if "reason" in (case.get("allowed_tools") or []):
             display = self._reasoning_display(prompt)
             if display:
                 required_text.extend([display["answer"], display["display"]])
@@ -161,19 +162,23 @@ def _build_generation_prompt(self, case: dict[str, Any], context: dict[str, Any]
         return "\n".join(line for line in lines if line)
 
     def _read_assets(self, values: Any) -> dict[str, Any]:
-        for value in values or []:
-            path = self._case_path(value)
-            if path.suffix.lower() != ".json":
-                continue
+        json_paths = [
+            path
+            for path in (self._case_path(value) for value in values or [])
+            if path.suffix.lower() == ".json"
+        ]
+        merged: dict[str, Any] = {"title": "", "sections": [], "required_text": []}
+        for path in sorted(json_paths, key=lambda item: item.name):
             data = json.loads(path.read_text(encoding="utf-8"))
             if not isinstance(data, dict):
                 continue
-            return {
-                "title": str(data.get("title") or ""),
-                "sections": [str(item) for item in data.get("sections", []) if str(item).strip()],
-                "required_text": [str(item) for item in data.get("required_text", []) if str(item).strip()],
+            merged = {
+                "title": str(data.get("title") or "") or merged["title"],
+                "sections": [str(item) for item in data.get("sections", []) if str(item).strip()] or merged["sections"],
+                "required_text": [str(item) for item in data.get("required_text", []) if str(item).strip()]
+                or merged["required_text"],
             }
-        return {"title": "", "sections": [], "required_text": []}
+        return merged
 
     def _read_search_facts(self, values: Any, prompt: str) -> list[str]:
         prompt_words = set(re.findall(r"[a-z0-9]+", prompt.lower()))
@@ -277,6 +282,10 @@ def _image_bytes_from_url(url: str, fallback_media_type: str, backend_config: di
         except ValueError as exc:
             raise OpenRouterImageError("OpenRouter image data URL included invalid base64 bytes") from exc
 
+    scheme = urllib.parse.urlparse(url).scheme.lower()
+    if scheme not in {"http", "https"}:
+        raise OpenRouterImageError(f"OpenRouter image URL scheme is not allowed: {scheme or url!r}")
+
     request = urllib.request.Request(url, method="GET")
     try:
         with urllib.request.urlopen(request, timeout=int(backend_config.get("timeout_seconds", 240))) as response:
@@ -320,7 +329,10 @@ def _extract_expression(prompt: str) -> str:
     for match in re.finditer(rf"(?<![A-Za-z0-9_.])(?:{number}|\()[0-9()\s+\-*/.]*", prompt):
         candidate = match.group(0).strip().rstrip(".")
         if any(char.isdigit() for char in candidate) and any(op in candidate for op in "+-*/"):
-            ast.parse(candidate, mode="eval")
+            try:
+                ast.parse(candidate, mode="eval")
+            except SyntaxError:
+                continue
             return candidate
     return ""
 

imagent-bench/src/imagent_bench/agent_loader.py

@@ -65,7 +65,11 @@ def load_agent(repository: Path) -> tuple[Any, dict[str, Any]]:
         raise AgentLoadError("agent manifest entrypoint must include module and attribute")
 
     _clear_candidate_modules(module_name)
-    sys.path.insert(0, str(repository))
+    repository_path = str(repository)
+    # Avoid unbounded sys.path growth across repeated loads; keep the entry so
+    # candidate agents can still perform lazy imports later.
+    if repository_path not in sys.path:
+        sys.path.insert(0, repository_path)
     try:
         module = importlib.import_module(module_name)
         agent_cls = getattr(module, attribute)

imagent-bench/src/imagent_bench/models.py

@@ -4,6 +4,14 @@
 from typing import Any
 
 
+def _safe_int(value: Any) -> int:
+    # Non-integer ids/seeds should not abort the whole run; fall back to 0.
+    try:
+        return int(value)
+    except (TypeError, ValueError):
+        return 0
+
+
 @dataclass(frozen=True)
 class BenchmarkConfig:
     benchmark_version: str
@@ -32,13 +40,13 @@ def from_record(cls, record: dict[str, Any]) -> "BenchmarkCase":
         raw_id = record.get("id") or record.get("run_id") or record.get("ID")
         if raw_id is None:
             raise ValueError("benchmark case is missing id")
-        numeric_id = int(record.get("ID", record.get("numeric_id", 0)) or 0)
+        numeric_id = _safe_int(record.get("ID", record.get("numeric_id", 0)) or 0)
         return cls(
             id=str(raw_id),
             numeric_id=numeric_id,
             prompt=str(record["prompt"]),
             capability=str(record.get("capability", "plan")),
-            seed=int(record.get("seed", 0)),
+            seed=_safe_int(record.get("seed", 0)),
             allowed_tools=[str(value) for value in record.get("allowed_tools", [])],
             expected=dict(record.get("expected", {}) or {}),
             assets=[str(value) for value in record.get("assets", []) or []],

imagent-bench/src/imagent_bench/runner.py

@@ -251,7 +251,9 @@ def _ranking(
         }
 
     delta = round(candidate_score - baseline_score, 6)
-    label = "score-regression"
+    # Only a negative delta is a regression. A positive or zero delta that is too
+    # small to clear the smallest improvement threshold is a non-regression.
+    label = "score-regression" if delta < 0 else "no-significant-change"
     for name, threshold in sorted(thresholds.items(), key=lambda item: item[1]):
         if delta >= threshold:
             label = f"improvement-{name}"
@@ -296,7 +298,10 @@ def _percentile(values: list[float], percentile: int) -> float:
     if len(values) == 1:
         return round(values[0], 3)
     ordered = sorted(values)
-    return round(float(statistics.quantiles(ordered, n=100, method="inclusive")[percentile - 1]), 3)
+    quantiles = statistics.quantiles(ordered, n=100, method="inclusive")
+    # quantiles has 99 cut points (indices 0..98); clamp so p<=0 and p=100 stay in range.
+    index = min(max(percentile - 1, 0), len(quantiles) - 1)
+    return round(float(quantiles[index]), 3)
 
 
 def _utc_now() -> str:
@@ -305,7 +310,8 @@ def _utc_now() -> str:
 
 def _append_log(path: Path, message: str) -> None:
     path.parent.mkdir(parents=True, exist_ok=True)
-    path.open("a", encoding="utf-8").write(json.dumps({"ts": _utc_now(), "message": message}) + "\n")
+    with path.open("a", encoding="utf-8") as f:
+        f.write(json.dumps({"ts": _utc_now(), "message": message}) + "\n")
 
 
 def _validate_local_repository_commit(repository: str | Path, requested_commit: str | None, actual_commit: str) -> None:
@@ -359,6 +365,7 @@ def _normalize_repository_identifier(value: str) -> str:
     text = str(value).strip()
     if not text:
         return ""
+    text = _strip_url_credentials(text)
     for pattern in (
         r"^https?://github\.com/(?P<owner>[A-Za-z0-9_.-]+)/(?P<repo>[A-Za-z0-9_.-]+?)(?:\.git)?/?$",
         r"^ssh://git@github\.com/(?P<owner>[A-Za-z0-9_.-]+)/(?P<repo>[A-Za-z0-9_.-]+?)(?:\.git)?/?$",
@@ -371,6 +378,21 @@ def _normalize_repository_identifier(value: str) -> str:
     return text.removesuffix(".git")
 
 
+def _strip_url_credentials(text: str) -> str:
+    # Remove embedded userinfo (e.g. tokens or user:pass) from a URL authority so
+    # credentials never leak into the normalized identifier or the report. Only
+    # scheme-based URLs carry userinfo before an "@" in the authority; the SCP
+    # form (git@github.com:owner/repo.git) has no "//" and is left untouched.
+    match = re.match(r"^(?P<scheme>[a-zA-Z][a-zA-Z0-9+.-]*://)(?P<rest>.*)$", text)
+    if not match:
+        return text
+    rest = match.group("rest")
+    authority, sep, tail = rest.partition("/")
+    if "@" in authority:
+        authority = authority.rsplit("@", 1)[1]
+    return f"{match.group('scheme')}{authority}{sep}{tail}"
+
+
 def _github_report_metadata() -> tuple[dict[str, Any] | None, dict[str, Any] | None]:
     payload = _github_event_payload()
     if not payload:

imagent-bench/src/imagent_bench/scoring.py

@@ -6,6 +6,7 @@
 import mimetypes
 import os
 import re
+import time
 import xml.etree.ElementTree as ET
 from pathlib import Path
 from typing import Any
@@ -37,6 +38,9 @@ def evaluate_case(
     judge_config = judge_config or {}
     provider = str(judge_config.get("provider", "mock_text")).strip().lower()
     if provider in {"openrouter", "openrouter_vision"}:
+        # Intentional design: when a vision judge is configured, deterministic
+        # image_contains checks are skipped in favor of the model's holistic score.
+        # This is not a bug; text-based checks can't reliably validate a raster image.
         judge_result = evaluate_openrouter_vision(image_path, prompt=prompt, config=judge_config)
         return [], float(judge_result["overall_score"]), judge_result
 
@@ -112,14 +116,33 @@ def evaluate_openrouter_vision(image_path: Path, *, prompt: str, config: dict[st
         headers=headers,
         method="POST",
     )
-    try:
-        with urllib.request.urlopen(request, timeout=int(config.get("timeout_seconds", 180))) as response:
-            response_payload = json.loads(response.read().decode("utf-8"))
-    except urllib.error.HTTPError as exc:
-        body = exc.read().decode("utf-8", errors="replace")
-        raise JudgeError(f"OpenRouter judge HTTP {exc.code}: {body}") from exc
-    except urllib.error.URLError as exc:
-        raise JudgeError(f"OpenRouter judge request failed: {exc}") from exc
+    timeout = int(config.get("timeout_seconds", 180))
+    max_retries = int(config.get("max_retries", 3))
+    delay_seconds = float(config.get("retry_backoff_seconds", 1.0))
+    response_payload: dict[str, Any] | None = None
+    # Retry transient failures (429/5xx and network errors) with exponential
+    # backoff, mirroring backends/openrouter_backend.py, so a single blip does
+    # not abort the whole benchmark.
+    for attempt in range(max_retries + 1):
+        try:
+            with urllib.request.urlopen(request, timeout=timeout) as response:
+                response_payload = json.loads(response.read().decode("utf-8"))
+            break
+        except urllib.error.HTTPError as exc:
+            if exc.code in {429, 500, 502, 503, 504} and attempt < max_retries:
+                time.sleep(delay_seconds)
+                delay_seconds *= 2
+                continue
+            body = exc.read().decode("utf-8", errors="replace")
+            raise JudgeError(f"OpenRouter judge HTTP {exc.code}: {body}") from exc
+        except urllib.error.URLError as exc:
+            if attempt < max_retries:
+                time.sleep(delay_seconds)
+                delay_seconds *= 2
+                continue
+            raise JudgeError(f"OpenRouter judge request failed: {exc}") from exc
+    if response_payload is None:  # pragma: no cover - defensive, loop always sets or raises
+        raise JudgeError("OpenRouter judge request failed after retries")
 
     content = _openrouter_message_content(response_payload)
     parsed = _parse_judge_json(content)
@@ -146,6 +169,9 @@ def evaluate_openrouter_vision(image_path: Path, *, prompt: str, config: dict[st
 
 
 def _read_visible_text(image_path: Path) -> str:
+    # Intentional design limitation: only .svg and .txt expose extractable text.
+    # Raster formats (.png/.jpg/...) return "" because text can't be recovered
+    # without OCR, which is out of scope here. This is not a bug.
     if not image_path.exists():
         return ""
     if image_path.suffix.lower() == ".svg":

imagent-bench/tests/test_agent_loader.py

@@ -0,0 +1,38 @@
+from __future__ import annotations
+
+import sys
+from pathlib import Path
+
+import pytest
+
+from imagent_bench.agent_loader import load_agent
+
+
+def _imagent_repository() -> Path:
+    workspace = Path(__file__).resolve().parents[2]
+    sibling_repo = workspace / "imagent"
+    if (sibling_repo / "agent" / "agent.py").exists():
+        return sibling_repo
+    if (workspace / "agent" / "agent.py").exists():
+        return workspace
+    return sibling_repo
+
+
+def test_load_agent_does_not_duplicate_sys_path_entries() -> None:
+    repository = _imagent_repository()
+    if not (repository / "agent" / "agent.py").exists():
+        pytest.skip("candidate imagent repository not available")
+
+    repository_path = str(repository)
+    before = sys.path.count(repository_path)
+
+    load_agent(repository)
+    after_first = sys.path.count(repository_path)
+
+    load_agent(repository)
+    after_second = sys.path.count(repository_path)
+
+    # A single load adds at most one entry, and a repeated load must never
+    # add a duplicate regardless of any pre-existing sys.path state.
+    assert after_first <= before + 1
+    assert after_second == after_first

imagent-bench/tests/test_models.py

@@ -0,0 +1,18 @@
+from __future__ import annotations
+
+from imagent_bench.models import BenchmarkCase
+
+
+def test_from_record_tolerates_non_integer_id_and_seed() -> None:
+    case = BenchmarkCase.from_record(
+        {
+            "id": "case-1",
+            "ID": "not-a-number",
+            "seed": "also-not-a-number",
+            "prompt": "draw a cat",
+        }
+    )
+
+    assert case.id == "case-1"
+    assert case.numeric_id == 0
+    assert case.seed == 0