From 61985d0ffc98a8e376a11394b598c80e13d93cee Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Thu, 6 Aug 2026 22:31:11 -0700 Subject: [PATCH 1/2] feat(eval): add a vision= kwarg to the harbor adapter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The vision_analyze tool (#800) is configured through the GLOBAL config's top-level `vision` block, and the harbor adapter had no way to seed it — so a container run could not use the tool at all. `fusion=` and `advisor=` both already ride the seeded config; this is the third of the same shape. Two halves, and the second is the one that fails quietly: --ak vision=openai:gpt-5.6-luna seeds `{"vision": {"enabled": true, "provider": ..., "model": ...}}` at the TOP LEVEL, not under `settings` — vision_config reads the global tier only, deliberately, because the key names the provider that receives image bytes. And it forwards that provider's key. Without it the model is offered a tool that cannot authenticate, and because Read's stub starts naming the tool the moment vision is configured, the failure reads like a tool bug rather than a missing credential. This is exactly the moonshot advisor gap: the run looks clean, the task can still score 1.0, and only a trajectory grep reveals it. For a worker+vision+advisor run the container now carries all three vendors' keys. Half-configured values (`openai:`, `:gpt-5.6-luna`) raise at construction rather than seeding a silently-inert config. Tests drive the real constructor rather than mirroring its check — a mirrored assertion passes even when the adapter drops validation entirely. Mutation-tested: removing the validation fails 4 of them. Added to the Harbor adapter CI job's file list, without which the file would never run at all. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 1 + eval/harbor/clawcodex_agent.py | 57 +++++++++- tests/test_harbor_adapter_vision.py | 156 ++++++++++++++++++++++++++++ 3 files changed, 213 insertions(+), 1 deletion(-) create mode 100644 tests/test_harbor_adapter_vision.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ba253d9e..b033bfea 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -83,6 +83,7 @@ jobs: tests/test_headless_usage_events.py \ tests/test_harbor_adapter_fusion.py \ tests/test_harbor_adapter_advisor.py \ + tests/test_harbor_adapter_vision.py \ -v event_file: diff --git a/eval/harbor/clawcodex_agent.py b/eval/harbor/clawcodex_agent.py index a06aba6b..4640e914 100644 --- a/eval/harbor/clawcodex_agent.py +++ b/eval/harbor/clawcodex_agent.py @@ -85,6 +85,14 @@ The name carries no ``/``, which leaves the env allowlist on its all-providers fallback — required here, since a fusion run legitimately needs two vendors' keys at once. +* ``vision`` — configure the ``vision_analyze`` TOOL, as + ``:`` (e.g. ``vision=openai:gpt-5.6-luna``). Gives a + TEXT-ONLY worker eyes it can aim: ``Read`` on an image returns a stub + naming the tool, and the model calls it with its own question. Distinct + from ``fusion``, which rewrites every image before the wire with one fixed + description and cannot be asked a follow-up. The two compose — fusion + covers images already in flight, this covers "look again, at the VAT line". + The vision provider's key is forwarded automatically. * ``max_turns`` — clawcodex ``--max-turns`` (default 300 here; the CLI's own default of 50 is too low for terminal-bench tasks). The ``CLAWCODEX_MAX_TURNS`` host env var works as a fallback. @@ -374,6 +382,7 @@ def __init__( forward_keys: bool | str = True, advisor: str | None = None, advisor_effort: str | None = None, + vision: str | None = None, *args, **kwargs, ): @@ -407,6 +416,21 @@ def __init__( "Agent kwarg 'advisor_effort' must be one of low|medium|" f"high|xhigh|max (got {self._advisor_effort!r})" ) + # ``vision`` is ``:`` — the model that answers the + # ``vision_analyze`` tool, letting a TEXT-ONLY worker ask targeted + # questions about an image. Distinct from ``fusion``: fusion rewrites + # every image before the wire with one fixed description, this is a + # tool the model calls on demand with its own question. Config, not a + # flag, so it rides the seeded global config like the advisor. + self._vision = vision + if self._vision: + provider_half, _, model_half = self._vision.partition(":") + if not provider_half.strip() or not model_half.strip(): + raise ValueError( + "Agent kwarg 'vision' must be ':' with " + f"both halves non-empty (got {self._vision!r}) — " + "e.g. openai:gpt-5.6-luna" + ) if self._advisor_effort and not self._advisor: raise ValueError( "Agent kwarg 'advisor_effort' requires 'advisor' — an effort " @@ -545,6 +569,25 @@ def _advisor_env_vars(self) -> tuple[str, ...]: keys = tuple(k for k in keys if k != "ANTHROPIC_API_KEY") return keys + def _vision_env_vars(self) -> tuple[str, ...]: + """Env keys the VISION provider needs, beyond the main loop's. + + ``vision_analyze`` makes its own API call to its own provider, so a + run whose eyes sit at a different vendor needs that vendor's key too. + Without this the tool is advertised, the model calls it, and it dies + on a missing key — and because ``Read``'s stub starts naming the tool + the moment vision is configured, the failure reads like a tool bug + rather than a credentials one. Exactly the shape of the moonshot + advisor gap that produced a clean-looking run with a dead advisor. + """ + if not self._vision: + return () + provider = self._vision.split(":", 1)[0].strip().lower() + keys = _PROVIDER_ENV_VARS.get(provider, _ALL_PROVIDER_ENV_VARS) + if self._subscription: + keys = tuple(k for k in keys if k != "ANTHROPIC_API_KEY") + return keys + def _build_env(self) -> dict[str, str]: env: dict[str, str] = {} if self._subscription: @@ -573,7 +616,9 @@ def _build_env(self) -> dict[str, str]: # The advisor calls its own provider, which may be a different # vendor than the worker's. Union, deduped, order-preserving. forwarded = tuple( - dict.fromkeys(forwarded + self._advisor_env_vars()) + dict.fromkeys( + forwarded + self._advisor_env_vars() + self._vision_env_vars() + ) ) for key in forwarded: value = self._get_env(key) @@ -766,6 +811,16 @@ async def _seed_container_settings( env_block = self._host_env_keys() if env_block: config["env"] = env_block + if self._vision: + vision_provider, vision_model = self._vision.split(":", 1) + # Top-level, NOT under settings: src/providers/vision_config.py + # reads the global tier only, deliberately — this key names the + # provider that receives image bytes. + config["vision"] = { + "enabled": True, + "provider": vision_provider.strip(), + "model": vision_model.strip(), + } fusion = self._fusion_record() if fusion: config["fusionModels"] = [fusion] diff --git a/tests/test_harbor_adapter_vision.py b/tests/test_harbor_adapter_vision.py new file mode 100644 index 00000000..9fb0d1f8 --- /dev/null +++ b/tests/test_harbor_adapter_vision.py @@ -0,0 +1,156 @@ +"""Vision-tool wiring in the harbor eval adapter. + +Runs ONLY in the dedicated "Harbor adapter (3.13)" CI job, which installs +harbor explicitly. ``eval/harbor/clawcodex_agent.py`` imports ``harbor`` at +module scope, so under the main ``test (3.11)`` job the ``importorskip`` +below fires and every assertion here skips silently. A file left out of that +job's file list therefore never runs at all — add new ``tests/test_harbor_*`` +files to it. + +The failure mode pinned here is the QUIET one this adapter keeps producing: +a ``vision_analyze`` whose provider key never reached the container is +advertised to the model, called, and dies on missing credentials — while the +worker carries on and the task can still score 1.0. The moonshot advisor gap +had exactly this shape and was found by grepping a container trajectory, not +by a red test. +""" + +from __future__ import annotations + +import asyncio +import json +import sys +from pathlib import Path + +import pytest + +pytest.importorskip("harbor", reason="harbor is an eval-only tool dependency") + +_ADAPTER_DIR = Path(__file__).resolve().parents[1] / "eval" / "harbor" +if str(_ADAPTER_DIR) not in sys.path: + sys.path.insert(0, str(_ADAPTER_DIR)) + +from clawcodex_agent import Clawcodex # noqa: E402 + + +def _agent( + *, + model_provider: str = "deepseek", + vision: str | None = None, + advisor: str | None = None, + subscription: bool = False, +) -> Clawcodex: + agent = Clawcodex.__new__(Clawcodex) + agent._subscription = subscription + agent._advisor = advisor + agent._advisor_effort = None + agent._vision = vision + agent._parsed_model_provider = model_provider + agent._forward_keys = False + agent._fusion = None + agent._resolved_flags = {"effort": "max"} + agent._extra_env = {} + agent._get_env = lambda key: f"{key}-VALUE" + return agent + + +def _seeded_config(agent: Clawcodex) -> dict: + captured: dict = {} + + async def fake_exec(environment, command=None, env=None): + captured.update(json.loads(env["CLAWCODEX_SEED_CONFIG"])) + + agent.exec_as_agent = fake_exec + agent._host_env_keys = lambda: {} + agent._fusion_record = lambda: None + asyncio.run(agent._seed_container_settings(None)) + return captured + + +# --------------------------------------------------------------------- config + + +def test_vision_block_is_seeded_at_the_top_level_not_in_settings() -> None: + """``vision_config`` reads the GLOBAL tier only, deliberately — the key + names the provider that receives image bytes. A ``settings`` key would + also merge from project/local tiers, which is the exposure that design + exists to avoid.""" + cfg = _seeded_config(_agent(vision="openai:gpt-5.6-luna")) + assert cfg["vision"] == { + "enabled": True, + "provider": "openai", + "model": "gpt-5.6-luna", + } + assert "vision" not in cfg.get("settings", {}) + + +def test_no_vision_block_when_unset() -> None: + assert "vision" not in _seeded_config(_agent()) + + +def test_vision_and_advisor_coexist() -> None: + cfg = _seeded_config(_agent(vision="openai:gpt-5.6-luna", advisor="moonshot:kimi-k3")) + assert cfg["vision"]["model"] == "gpt-5.6-luna" + assert cfg["settings"]["advisor_model"] == "kimi-k3" + assert cfg["settings"]["advisor_enabled"] is True + + +# ---------------------------------------------------------------- credentials + + +def test_vision_provider_key_is_forwarded() -> None: + """The tool calls its OWN provider. Without this the model is offered a + tool that cannot authenticate — and Read's stub starts naming it, so the + failure reads like a tool bug rather than a missing key.""" + env = _agent(model_provider="deepseek", vision="openai:gpt-5.6-luna")._build_env() + assert "DEEPSEEK_API_KEY" in env, "the worker still needs its own key" + assert "OPENAI_API_KEY" in env + + +def test_worker_vision_and_advisor_keys_are_all_forwarded() -> None: + """The three-vendor configuration this was built for.""" + env = _agent( + model_provider="deepseek", + vision="openai:gpt-5.6-luna", + advisor="moonshot:kimi-k3", + )._build_env() + for key in ("DEEPSEEK_API_KEY", "OPENAI_API_KEY", "MOONSHOT_API_KEY"): + assert key in env, key + + +def test_anthropic_key_withheld_when_vision_is_anthropic_under_subscription() -> None: + """OAuth must stay the only route to the subscription: inside clawcodex + an API key outranks it and would silently bill the API instead.""" + env = _agent( + model_provider="deepseek", + vision="anthropic:claude-opus-5", + subscription=True, + )._build_env() + assert "ANTHROPIC_API_KEY" not in env + assert "DEEPSEEK_API_KEY" in env + + +# ----------------------------------------------------------------- validation + + +@pytest.mark.parametrize("bad", ["openai:", ":gpt-5.6-luna", "openai", " : "]) +def test_half_configured_vision_is_rejected_at_construction(bad: str) -> None: + """Fail loudly at setup rather than seeding a silently-inert config — + the failure mode this adapter keeps producing. + + Drives the REAL constructor, not a mirror of its check: a mirrored + assertion passes even if the adapter drops the validation entirely. + """ + import tempfile + + with pytest.raises(ValueError, match="vision"): + Clawcodex(logs_dir=Path(tempfile.mkdtemp()), vision=bad) + + +def test_well_formed_vision_is_accepted() -> None: + import tempfile + + agent = Clawcodex( + logs_dir=Path(tempfile.mkdtemp()), vision="openai:gpt-5.6-luna" + ) + assert agent._vision == "openai:gpt-5.6-luna" From 44fe571edbfc8ac7705ab24d3367f5df17f07c5a Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Thu, 6 Aug 2026 22:41:10 -0700 Subject: [PATCH 2/2] fix(eval): mirror the vision field in the advisor test's agent factory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _build_env now consults self._vision, and the advisor tests build a Clawcodex via __new__ with a hand-listed field set — so every one of them raised AttributeError. The factory models a constructed agent and has to track the constructor. Caught by CI, not locally: `pytest -k harbor` in the main venv SKIPS these files (importorskip("harbor")), so a local green tick said nothing about them. The Harbor adapter job's exact command is the one to run: uv run --isolated --python 3.13 --with harbor --with pytest python -m pytest \ tests/test_headless_usage_events.py tests/test_harbor_adapter_*.py 62 passed. Co-Authored-By: Claude Opus 5 --- tests/test_harbor_adapter_advisor.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_harbor_adapter_advisor.py b/tests/test_harbor_adapter_advisor.py index 0bec98a2..7070d821 100644 --- a/tests/test_harbor_adapter_advisor.py +++ b/tests/test_harbor_adapter_advisor.py @@ -44,6 +44,9 @@ def _agent( agent = Clawcodex.__new__(Clawcodex) agent._subscription = subscription agent._advisor = advisor + # Constructed agents always carry this; the helper must mirror the + # constructor or _build_env raises AttributeError on the vision path. + agent._vision = None agent._advisor_effort = advisor_effort agent._parsed_model_provider = model_provider agent._forward_keys = False