diff --git a/.github/integration/scope_map.yml b/.github/integration/scope_map.yml index fbef5d2b8..3ca3b4df7 100644 --- a/.github/integration/scope_map.yml +++ b/.github/integration/scope_map.yml @@ -106,8 +106,6 @@ roster: affected_agent_map: - glob: "src/benchflow/agents/codex_config.py" agent: codex-acp - - glob: "src/benchflow/agents/openclaw_acp_shim.py" - agent: openclaw - glob: "src/benchflow/agents/pi_acp_launcher.py" agent: pi-acp # Generic per-agent source files: src/benchflow/agents/*.py where diff --git a/docs/external-agents.md b/docs/external-agents.md index 156ca4fe5..551ca1f8f 100644 --- a/docs/external-agents.md +++ b/docs/external-agents.md @@ -3,7 +3,7 @@ BenchFlow's built-in registry covers a handful of agents (`bench agent list`). Everything else — goose, qwen-code, prime-agent, the omnigent harnesses, … — lives in the public **[benchflow-ai/agents](https://github.com/benchflow-ai/agents)** -repo and loads into BenchFlow through one of four paths. For most users the +repo and loads into BenchFlow through one of three paths. For most users the first one is all there is to know. ## 1. Zero-config remote autoload (the default) @@ -65,6 +65,7 @@ directory, or off entirely: ```bash export BENCHFLOW_AGENTS_SOURCE="benchflow-ai/agents@my-branch" # owner/repo[@ref] +export BENCHFLOW_AGENTS_SOURCE="benchflow-ai/agents@0123456789abcdef0123456789abcdef01234567" # reproducible export BENCHFLOW_AGENTS_SOURCE="/path/to/agents-checkout" # local dir export BENCHFLOW_AGENTS_SOURCE="off" # disable autoload ``` @@ -74,21 +75,27 @@ the standard way to try an agent from an open PR — e.g. verified live on BenchFlow 0.6.6: `BENCHFLOW_AGENTS_SOURCE="benchflow-ai/agents@add-prime-agent"` resolved and ran the `prime-agent` manifest with zero local setup. -## 3. Local checkout at import: `BENCHFLOW_AGENTS_DIR` +`bench agent list` reads this catalog best-effort and reports one consolidated +warning when it is incomplete. `bench agent show NAME` uses runtime resolution. +Unknown bare IDs fail closed. Raw commands require explicit syntax: whitespace +(`agent --flag`) or a path prefix (`/`, `./`, `../`, or `~/`). Remote refs are +fetched/refreshed by the normal source resolver; there is no offline fallback. -For agents-repo development: point at a checkout and every -`/manifest.toml` under it merges into the registry when `benchflow` -imports (not lazily on miss): +## 3. Local checkout override: `BENCHFLOW_AGENTS_DIR` + +For agents-repo development, point at a checkout. This selects the same lazy +catalog path used by `BENCHFLOW_AGENTS_SOURCE`: ```bash export BENCHFLOW_AGENTS_DIR=/path/to/agents-checkout ``` -Unlike the miss-driven autoload, this path loads even for names that would -never miss, and it is the loop used while editing a manifest. It is additive -and compatible-merge only: colliding with an existing agent's aliases is a -hard error rather than a silent shadow. Unset, the import is byte-for-byte -identical to core — the mechanism is strictly opt-in. +When both variables are set, a nonblank `BENCHFLOW_AGENTS_DIR` wins. Its local +override mode may update an unchanged built-in's manifest-owned fields, but +never replaces a plugin/runtime-modified entry. Loading remains one-shot after +first runtime resolution. `bench agent list` may preview the selected catalog +without activating or caching it; after activation, listing reuses the applied +result. ## 4. Plugin packages (entry points) @@ -109,17 +116,11 @@ error message if its name is later requested. ## Precedence 1. Built-in registry (core `AGENTS`). -2. `BENCHFLOW_AGENTS_DIR` manifests — merged at import; a collision with an - existing agent name or alias is a hard error, so manifests never shadow - built-ins. -3. Entry-point plugin packages — loaded at import, after the manifest merge. - These register through plain `register_agent`, which overwrites by name: - a plugin **can** replace a built-in (or manifest-registered) agent that - shares its name. Well-behaved plugins skip names the registry already owns - (as the acp-registry package does). -4. Remote autoload (`BENCHFLOW_AGENTS_SOURCE`, default `benchflow-ai/agents@main`) - — consulted last, once, only for names still unknown at resolution time; - it fills gaps and never overwrites. +2. Entry-point plugin packages, loaded at import. Plugins may replace built-ins. +3. One lazily selected manifest catalog. Nonblank `BENCHFLOW_AGENTS_DIR` wins + over `BENCHFLOW_AGENTS_SOURCE` and uses local-override policy; otherwise + remote/default loading uses gap-fill policy and never replaces existing + entries. Manifest capabilities are deliberately bounded: a `manifest.toml` is data-only (install/launch commands, env mapping, model-routing hints — the diff --git a/docs/reference/python-api.md b/docs/reference/python-api.md index 2f2cdda9a..d3e9a4e4d 100644 --- a/docs/reference/python-api.md +++ b/docs/reference/python-api.md @@ -263,7 +263,7 @@ config = rollout_config_from_yaml("rollout.yaml") result = await bf.run(config) ``` -## Registered Agents +## Built-in Agents | Agent | Protocol | Auth | Aliases | |-------|----------|------|---------| @@ -273,7 +273,6 @@ result = await bf.run(config) | `opencode` | ACP | inferred from model/provider | — | | `openhands` | ACP | LLM_API_KEY | `oh` | | `pi-acp` | ACP | ANTHROPIC_API_KEY | `pi` | -| `openclaw` | ACP | inferred from model | — | The Auth column shows each agent's native/default credentials. Provider-prefixed models can use provider-specific credentials instead; for example, Azure @@ -282,6 +281,9 @@ as `azure-foundry-openai/gpt-5.5` or `azure-foundry-anthropic/claude-opus-4-5`. BenchFlow routes these providers through LiteLLM on both Docker and Daytona. +Additional agents load lazily from the external agents catalog. See +[External agents](../external-agents.md). + Any agent can be prefixed with `acpx/` to run via [ACPX](https://acpx.sh/) (e.g. `acpx/gemini`, `acpx/claude`). ACPX is a headless ACP client with persistent sessions and crash recovery. The underlying agent's install, env, credentials, and skill paths are preserved. ## Retry and Error Handling diff --git a/src/benchflow/_utils/config.py b/src/benchflow/_utils/config.py index 54fd4c1ea..ccb309719 100644 --- a/src/benchflow/_utils/config.py +++ b/src/benchflow/_utils/config.py @@ -13,7 +13,7 @@ def normalize_agent_name(agent: str) -> str: stable runtime key is returned, so the Rollout/Evaluation path resolves acpx install/launch commands instead of the literal spec string. - Unknown specs are returned unchanged. + Unknown bare IDs fail closed. Explicit raw commands pass through unchanged. """ return resolve_agent_key(agent) diff --git a/src/benchflow/acp/client.py b/src/benchflow/acp/client.py index 98d033766..89f9313b9 100644 --- a/src/benchflow/acp/client.py +++ b/src/benchflow/acp/client.py @@ -330,7 +330,7 @@ async def session_load( cwd: str = "/app", mcp_servers: list[McpServerSpec] | None = None, ) -> ACPSession: # ACP spec; unused until session resume is wired - """Load an existing session (used by agents like openclaw that need pre-created sessions). + """Load an existing pre-created session. ``mcp_servers`` mirrors :meth:`session_new` — the same task-configured servers are attached to the resumed session. diff --git a/src/benchflow/acp/runtime.py b/src/benchflow/acp/runtime.py index 1af7895c8..6744cf69d 100644 --- a/src/benchflow/acp/runtime.py +++ b/src/benchflow/acp/runtime.py @@ -571,16 +571,19 @@ async def _configure_acp_session( f"Reasoning effort {reasoning_effort!r} applied with model selection for {agent}" ) return - if not agent_cfg or not agent_cfg.acp_effort_config_id: + effort_config_id = getattr(agent_cfg, "acp_effort_config_id", "") or ( + "effort" if "effort" in _session_config_option_ids(session) else None + ) + if not effort_config_id: raise RuntimeError( f"reasoning_effort={reasoning_effort!r} was requested for agent " - f"{agent!r}, but that agent does not declare an ACP effort config option" + f"{agent!r}, but that agent does not declare or advertise an ACP effort config option" ) await _set_acp_config_option( acp_client, session, agent=agent, - config_id=agent_cfg.acp_effort_config_id, + config_id=effort_config_id, value=reasoning_effort, label="reasoning effort", ) diff --git a/src/benchflow/acp/session.py b/src/benchflow/acp/session.py index 02387a93a..7192e7d2e 100644 --- a/src/benchflow/acp/session.py +++ b/src/benchflow/acp/session.py @@ -469,14 +469,14 @@ def handle_update(self, update: dict) -> None: self._pending_text.append({"type": "agent_message", "text": text}) elif update_type == "text_update": - # Used by openclaw shim — full text (not chunked) + # Some ACP agents send full text rather than chunks. text = update.get("text", "") if text: self.message_chunks.append(text) self._pending_text.append({"type": "agent_message", "text": text}) elif update_type == "agent_thought": - # Used by openclaw shim — full thought (not chunked) + # Some ACP agents send full thoughts rather than chunks. text = update.get("text", "") if text: self.thought_chunks.append(text) diff --git a/src/benchflow/agents/__init__.py b/src/benchflow/agents/__init__.py index 1ac2d057d..55af96560 100644 --- a/src/benchflow/agents/__init__.py +++ b/src/benchflow/agents/__init__.py @@ -16,11 +16,6 @@ native LLM providers, ``base_url`` / ``url_params`` resolution, ADC handling. The "add a new provider" recipe lives in the module docstring. -- ``openclaw_acp_shim.py`` Standalone script (read at import time by - ``registry.py``) that wraps ``openclaw agent - --local`` as an ACP server over stdio. Needed - because openclaw's native ACP bridge requires a - gateway. Nothing is re-exported from this ``__init__``: importers go through ``benchflow.agents.registry`` / ``benchflow.agents.providers`` directly, which is what the registry-only-change rule depends on. diff --git a/src/benchflow/agents/env.py b/src/benchflow/agents/env.py index ffa5e5f02..8d4d68e38 100644 --- a/src/benchflow/agents/env.py +++ b/src/benchflow/agents/env.py @@ -484,7 +484,7 @@ def resolve_provider_env( # Resolve bare family ids (e.g. "deepseek-v4-pro") too, not just explicit # "provider/" prefixes — otherwise the provider env (NAME/BASE_URL/API_KEY) # is never emitted and harnesses that rely on it misroute (openhands' litellm - # saw no provider; openclaw defaulted to anthropic/). Mirrors acp/runtime.py. + # saw no provider and defaulted incorrectly). Mirrors acp/runtime.py. _prov = find_provider(model) or find_provider_for_bare_model(model) if _prov: _prov_name, _prov_cfg = _prov diff --git a/src/benchflow/agents/manifest.py b/src/benchflow/agents/manifest.py index daebc8cc0..da5d4c856 100644 --- a/src/benchflow/agents/manifest.py +++ b/src/benchflow/agents/manifest.py @@ -24,11 +24,12 @@ from __future__ import annotations -import os import re import tomllib +from collections import Counter from collections.abc import Mapping from dataclasses import dataclass, replace +from enum import StrEnum from pathlib import Path from benchflow.agents.registry import AgentConfig @@ -44,12 +45,22 @@ # skipping registry.VALID_PROTOCOLS — so this loader is the last protocol gate. _SUPPORTED_PROTOCOLS = frozenset({"acp"}) -# Directory env override for opt-in filesystem discovery (decision #7). -MANIFEST_DIR_ENV = "BENCHFLOW_AGENTS_DIR" - _REQUIRED = ("contract_version", "name", "install_cmd", "launch_cmd") _VERSION_RE = re.compile(r"^\d+\.\d+(\.\d+)?$") + +class ManifestIssueKind(StrEnum): + """Stable categories for manifest loading and resolution failures.""" + + DISABLED = "disabled" + UNREACHABLE = "unreachable" + MALFORMED = "malformed" + INCOMPATIBLE = "incompatible" + MISSING = "missing" + DUPLICATE = "duplicate" + COLLISION = "collision" + + # manifest key -> AgentConfig field: the contract's data fields (the PR #14 schema # minus the meta keys contract_version/aliases). The consumer must cover EVERY # schema data field with an AgentConfig home; _SHIM_ONLY below holds the rest. @@ -112,6 +123,18 @@ class AgentManifestError(ValueError): """A manifest.toml is unreadable, missing a required field, declares an unsupported contract major version, or collides with an existing agent.""" + def __init__( + self, + detail: str, + *, + kind: ManifestIssueKind = ManifestIssueKind.MALFORMED, + path: str = "", + ) -> None: + self.kind = kind + self.path = path + self.detail = detail + super().__init__(detail) + @dataclass(frozen=True) class LoadedManifest: @@ -132,7 +155,8 @@ def _check_contract_version(raw: object) -> None: if major != SUPPORTED_CONTRACT_MAJOR: raise AgentManifestError( f"contract_version {raw!r} declares major {major}; this loader speaks " - f"contract {SUPPORTED_CONTRACT_MAJOR}.x" + f"contract {SUPPORTED_CONTRACT_MAJOR}.x", + kind=ManifestIssueKind.INCOMPATIBLE, ) @@ -146,11 +170,22 @@ def load_agent_manifest(path: str | Path) -> LoadedManifest: try: data = tomllib.loads(path.read_text()) except (OSError, tomllib.TOMLDecodeError) as exc: - raise AgentManifestError(f"cannot read manifest {path}: {exc}") from exc + raise AgentManifestError( + f"cannot read manifest {path}: {exc}", path=str(path) + ) from exc for key in _REQUIRED: if key not in data: raise AgentManifestError(f"manifest {path} is missing required {key!r}") + if not isinstance(data["name"], str): + raise AgentManifestError(f"manifest {path}: name must be a string") + raw_aliases = data.get("aliases", []) + if not isinstance(raw_aliases, list) or not all( + isinstance(alias, str) for alias in raw_aliases + ): + raise AgentManifestError( + f"manifest {path}: aliases must be an array of strings" + ) _check_contract_version(data["contract_version"]) protocol = data.get("protocol", "acp") @@ -167,7 +202,7 @@ def load_agent_manifest(path: str | Path) -> LoadedManifest: # wants seconds as an int. kwargs["install_timeout"] = int(kwargs["install_timeout"]) - aliases = tuple(data.get("aliases", ())) + aliases = tuple(raw_aliases) return LoadedManifest(config=AgentConfig(**kwargs), aliases=aliases) @@ -227,6 +262,63 @@ def load_agents_from_dir(root: str | Path) -> dict[str, LoadedManifest]: return out +def select_manifest_agents( + loaded: Mapping[str, LoadedManifest], + *, + agents: Mapping[str, AgentConfig], + aliases: Mapping[str, str], +) -> tuple[dict[str, LoadedManifest], list[tuple[str, ManifestIssueKind, str]]]: + """Select gap-filling manifests and describe rejected names or aliases.""" + name_counts = Counter(manifest.config.name for manifest in loaded.values()) + conflicts: list[tuple[str, ManifestIssueKind, str]] = [] + eligible: list[tuple[str, LoadedManifest]] = [] + for path, manifest in sorted(loaded.items()): + name = manifest.config.name + if name_counts[name] > 1: + conflicts.append( + (path, ManifestIssueKind.DUPLICATE, f"duplicate agent name {name!r}") + ) + elif name in agents or (name in aliases and aliases[name] != name): + conflicts.append( + ( + path, + ManifestIssueKind.COLLISION, + f"agent name {name!r} collides with local registry", + ) + ) + else: + eligible.append((path, manifest)) + + incoming_names = {manifest.config.name for _, manifest in eligible} + alias_owners: dict[str, set[str]] = {} + for path, manifest in eligible: + for alias in manifest.aliases: + alias_owners.setdefault(alias, set()).add(path) + + selected: dict[str, LoadedManifest] = {} + for path, manifest in eligible: + name = manifest.config.name + kept_aliases: list[str] = [] + for alias in dict.fromkeys(manifest.aliases): + if alias != name and ( + alias in agents + or aliases.get(alias, name) != name + or alias in incoming_names + or len(alias_owners[alias]) > 1 + ): + conflicts.append( + ( + path, + ManifestIssueKind.COLLISION, + f"alias {alias!r} collides with catalog or registry", + ) + ) + else: + kept_aliases.append(alias) + selected[name] = LoadedManifest(manifest.config, tuple(kept_aliases)) + return selected, conflicts + + def register_manifest_agents( loaded: Mapping[str, LoadedManifest], *, @@ -257,49 +349,21 @@ def register_manifest_agents( merged config equals the original. Alias collisions still fail loud because remapping another agent's alias is not part of the compatibility shim.""" if not override: - incoming_names = set(loaded) - seen_aliases: dict[str, str] = {} - for name, lm in loaded.items(): - if name in agents and not merge_shim_only: - raise AgentManifestError( - f"agent {name!r} already in the registry; ship its manifest as " - "the sole source or pass override=True" - ) - # Precedence: the name-vs-existing-name check above is exempted by - # merge_shim_only (a deliberate shim reproducing a core agent's own - # name). The name-vs-alias checks below are NOT — alias collisions - # always fail loud (see docstring). registry.py resolves alias-first - # (name = AGENT_ALIASES.get(name, name)), so a manifest NAME equal to - # an existing alias for a *different* agent is silently shadowed - # (unreachable); reject it even in shim mode. - shadow = aliases.get(name) - if shadow is not None and shadow != name: - raise AgentManifestError( - f"agent {name!r} collides with an existing alias mapping to " - f"{shadow!r}; it would be silently shadowed by alias resolution" - ) - for alias in lm.aliases: - existing = aliases.get(alias) - if existing is None: - existing = seen_aliases.get(alias) - if existing is not None and existing != name: - raise AgentManifestError( - f"alias {alias!r} (for {name!r}) already maps to {existing!r}" - ) - if alias in agents and alias != name: - raise AgentManifestError( - f"alias {alias!r} (for {name!r}) collides with an existing " - "agent name" - ) - # Same-batch cross-collision: an alias equal to *another* incoming - # agent's name would shadow that agent once both register. Checked - # against the whole incoming name set so it is order-independent. - if alias in incoming_names and alias != name: - raise AgentManifestError( - f"alias {alias!r} (for {name!r}) collides with another " - "agent name in the same batch" - ) - seen_aliases[alias] = name + existing_agents = agents + if merge_shim_only: + incoming_names = {manifest.config.name for manifest in loaded.values()} + existing_agents = { + name: config + for name, config in agents.items() + if name not in incoming_names + } + selected, conflicts = select_manifest_agents( + loaded, agents=existing_agents, aliases=aliases + ) + if conflicts: + path, kind, detail = conflicts[0] + raise AgentManifestError(detail, kind=kind, path=path) + loaded = selected for name, lm in loaded.items(): config = lm.config if merge_shim_only and name in agents: @@ -309,49 +373,3 @@ def register_manifest_agents( launch[name] = config.launch_cmd for alias in lm.aliases: aliases[alias] = name - - -def register_env_manifest_agents( - *, - agents: dict[str, AgentConfig] | None = None, - aliases: dict[str, str] | None = None, - installers: dict[str, str] | None = None, - launch: dict[str, str] | None = None, -) -> list[str]: - """Register agents from the directory named by ``$BENCHFLOW_AGENTS_DIR`` into - the registry; return the sorted names registered. - - A no-op returning ``[]`` when the env var is unset — so a default import of - core is unchanged and the dual-source registry only activates on explicit - opt-in. The four maps default to the live ``registry`` globals (resolved - lazily to avoid an import cycle); tests pass throwaway dicts to stay - hermetic.""" - root = os.environ.get(MANIFEST_DIR_ENV) - if not root: - return [] - # Resolve each map to the live registry globals when not supplied (lazy - # import to avoid the registry↔manifest cycle). Per-variable ``is None`` - # narrowing — not a tuple-membership check — so each is a concrete dict here. - from benchflow.agents.registry import ( - AGENT_ALIASES, - AGENT_INSTALLERS, - AGENT_LAUNCH, - AGENTS, - ) - - resolved_agents = AGENTS if agents is None else agents - resolved_aliases = AGENT_ALIASES if aliases is None else aliases - resolved_installers = AGENT_INSTALLERS if installers is None else installers - resolved_launch = AGENT_LAUNCH if launch is None else launch - loaded = load_agents_from_dir(root) - register_manifest_agents( - loaded, - agents=resolved_agents, - aliases=resolved_aliases, - installers=resolved_installers, - launch=resolved_launch, - # Additive/compatible: a manifest reproducing a core agent overrides it but - # keeps the core entry's host-side _SHIM_ONLY fields (subscription_auth, ...). - merge_shim_only=True, - ) - return sorted(loaded) diff --git a/src/benchflow/agents/openclaw_acp_shim.py b/src/benchflow/agents/openclaw_acp_shim.py deleted file mode 100644 index 359aff5e6..000000000 --- a/src/benchflow/agents/openclaw_acp_shim.py +++ /dev/null @@ -1,973 +0,0 @@ -#!/usr/bin/env python3 -"""ACP shim for OpenClaw — wraps `openclaw agent --local` as an ACP server. - -openclaw's native ACP bridge requires a gateway with chat-thread sessions. -This shim speaks ACP on stdio and internally calls `openclaw agent --local` -for each prompt, then parses openclaw's session JSONL to emit proper ACP -tool_call and text updates. - -Architecture: - benchflow ACP client ←stdio→ this shim ←subprocess→ openclaw agent --local - ←file read→ ~/.openclaw/agents/main/sessions/*.jsonl - -Key details: - - Workspace: symlinks ~/.openclaw/workspace → task cwd (openclaw ignores subprocess cwd) - - Skills: if task env has ~/.claude/skills/, also copies to ~/.openclaw/workspace/.claude/skills/ - - Trajectory: parses session JSONL for tool calls, thinking, text → emits ACP session/update - - Model: set via openclaw config on session/set_model -""" - -import base64 -import json -import logging -import os -import re -import shutil -import subprocess -import sys -import tempfile -import time -import urllib.parse -import urllib.request -from pathlib import Path -from typing import TYPE_CHECKING - -if TYPE_CHECKING: - # Type-only; the shim must stay runnable without benchflow installed. - from benchflow.agents.providers import ProviderConfig - -logger = logging.getLogger(__name__) - -_DIAG_TRUNCATE = 2000 # max chars for diagnostic output in ACP updates -_TOOL_RESULT_TRUNCATE = 1000 # max chars for tool result text -_TOOL_INPUT_TRUNCATE = 500 # max chars for tool input echoed in ACP updates -_OPENCLAW_BIN = "/opt/benchflow/bin/openclaw" - -_PARAM_MAP = { - "BENCHFLOW_MODEL_TEMPERATURE": "agents.defaults.params.temperature", - "BENCHFLOW_MODEL_TOP_P": "agents.defaults.params.topP", - "BENCHFLOW_MODEL_MAX_TOKENS": "agents.defaults.params.maxTokens", -} - - -_TOKEN_CAP_MODEL = ( - r"(?:(?:(?:openai|us-openai|azure-foundry-openai)/|" - r"benchflow-(?:(?:openai|us-openai|azure-foundry-openai)-)?)?gpt-5\.4|" - r"(?:(?:anthropic|anthropic-vertex|azure-foundry-anthropic)/|" - r"benchflow-(?:(?:anthropic|anthropic-vertex|azure-foundry-anthropic)-)?)?" - r"claude-(?:sonnet-4-6|opus-4-[6-8]|(?:sonnet|opus)-5(?:-\d+)?))" -) - - -def _default_max_tokens(model: str) -> int | None: - # Assume future numeric Sonnet/Opus 5.x releases retain the documented - # 128k max output; narrow/update the pattern if that limit changes. - return 128000 if re.fullmatch(_TOKEN_CAP_MODEL, model) else None - - -def _max_tokens_value(model: str, configured: str | None) -> str | None: - cap = _default_max_tokens(model) - if cap is None: - return configured - try: - value = int(configured or "") - except ValueError: - return str(cap) - return configured if 0 < value <= cap else str(cap) - - -# ── ACP stdio I/O ───────────────────────────────────────────────────────────── - - -def send(msg): - sys.stdout.write(json.dumps(msg) + "\n") - sys.stdout.flush() - - -def recv(): - while True: - line = sys.stdin.readline() - if not line: - raise EOFError("stdin closed") - line = line.strip() - if not line: - continue - return json.loads(line) - - -# ── Workspace + auth setup ──────────────────────────────────────────────────── - - -def setup_workspace(cwd: str): - """Point openclaw's workspace at the task directory and load skills. - - openclaw discovers skills from /skills/ (not .claude/skills/). - SkillsBench tasks bake skills into ~/.claude/skills/ via Dockerfile. - We symlink/copy them to /skills/ so openclaw can find them. - """ - home = os.environ.get("HOME", os.path.expanduser("~")) - oc_workspace = Path(home) / ".openclaw" / "workspace" - - if oc_workspace.is_symlink() or oc_workspace.exists(): - if oc_workspace.is_symlink(): - oc_workspace.unlink() - elif oc_workspace.is_dir(): - shutil.rmtree(oc_workspace) - - oc_workspace.parent.mkdir(parents=True, exist_ok=True) - oc_workspace.symlink_to(cwd) - - # Load skills: check common skill locations and copy to /skills/ - workspace_skills = Path(cwd) / "skills" - if not workspace_skills.exists(): - # Search for skills in known locations - skill_sources = [ - Path(cwd) / ".claude" / "skills", # SkillsBench Claude format - Path(home) / ".claude" / "skills", # Home dir Claude skills - Path(cwd) / ".codex" / "skills", # Codex format - Path(cwd) / ".agents" / "skills", # Generic agent skills - ] - for src in skill_sources: - if src.is_dir() and any(src.iterdir()): - # Copy skills to workspace/skills/ for openclaw discovery - workspace_skills.mkdir(parents=True, exist_ok=True) - for skill_dir in src.iterdir(): - if skill_dir.is_dir() and (skill_dir / "SKILL.md").exists(): - dest = workspace_skills / skill_dir.name - if not dest.exists(): - shutil.copytree(skill_dir, dest) - break # Use first source found - - -def setup_openai_auth(): - """Write OPENAI_API_KEY into openclaw's native auth store if present.""" - api_key = os.environ.get("OPENAI_API_KEY") - if not api_key: - return - agent_dir = Path.home() / ".openclaw" / "agents" / "main" / "agent" - auth_path = agent_dir / "auth-profiles.json" - existing = {} - if auth_path.exists(): - try: - existing = json.loads(auth_path.read_text()) - except (json.JSONDecodeError, OSError): - logger.debug("Could not read existing auth config at %s", auth_path) - existing["openai"] = {"apiKey": api_key} - agent_dir.mkdir(parents=True, exist_ok=True) - auth_path.write_text(json.dumps(existing)) - - -def setup_gcloud_adc(): - """Write ADC credentials from env var to disk and enable google plugin for Vertex AI.""" - adc_json = os.environ.get("GOOGLE_APPLICATION_CREDENTIALS_JSON") - if not adc_json: - return - adc_path = ( - Path.home() / ".config" / "gcloud" / "application_default_credentials.json" - ) - adc_path.parent.mkdir(parents=True, exist_ok=True) - adc_path.write_text(adc_json) - os.environ["GOOGLE_APPLICATION_CREDENTIALS"] = str(adc_path) - # Enable the google plugin so openclaw recognizes google-vertex/ models - subprocess.run( - [_OPENCLAW_BIN, "plugins", "enable", "google"], - capture_output=True, - timeout=10, - ) - - -def _get_adc_token() -> str: - """Get a bearer token from ADC credentials (stdlib only, no google-auth dep). - - Supports both service-account keys (JWT → token exchange) and - authorized-user credentials (refresh_token → token exchange). - """ - adc_path = os.environ.get("GOOGLE_APPLICATION_CREDENTIALS") - if not adc_path or not Path(adc_path).exists(): - # Fallback to default ADC location - adc_path = str( - Path.home() / ".config" / "gcloud" / "application_default_credentials.json" - ) - with open(adc_path) as f: - creds = json.load(f) - - cred_type = creds.get("type", "") - - if cred_type == "authorized_user": - # Refresh token flow - data = urllib.parse.urlencode( - { - "client_id": creds["client_id"], - "client_secret": creds["client_secret"], - "refresh_token": creds["refresh_token"], - "grant_type": "refresh_token", - } - ).encode() - req = urllib.request.Request( - "https://oauth2.googleapis.com/token", - data=data, - headers={"Content-Type": "application/x-www-form-urlencoded"}, - ) - with urllib.request.urlopen(req, timeout=30) as resp: - return json.loads(resp.read())["access_token"] - - elif cred_type == "service_account": - # JWT → access token flow (RS256) - # Requires PyJWT or manual RSA — use subprocess openssl as fallback - now = int(time.time()) - header = base64.urlsafe_b64encode( - json.dumps({"alg": "RS256", "typ": "JWT"}).encode() - ).rstrip(b"=") - payload = base64.urlsafe_b64encode( - json.dumps( - { - "iss": creds["client_email"], - "scope": "https://www.googleapis.com/auth/cloud-platform", - "aud": "https://oauth2.googleapis.com/token", - "iat": now, - "exp": now + 3600, - } - ).encode() - ).rstrip(b"=") - signing_input = header + b"." + payload - - # Sign with openssl (available in most containers) - with tempfile.NamedTemporaryFile(mode="w", suffix=".pem", delete=False) as kf: - kf.write(creds["private_key"]) - key_path = kf.name - try: - result = subprocess.run( - ["openssl", "dgst", "-sha256", "-sign", key_path], - input=signing_input, - capture_output=True, - timeout=10, - ) - signature = base64.urlsafe_b64encode(result.stdout).rstrip(b"=") - finally: - os.unlink(key_path) - - jwt_token = (signing_input + b"." + signature).decode() - data = urllib.parse.urlencode( - { - "grant_type": "urn:ietf:params:oauth:grant-type:jwt-bearer", - "assertion": jwt_token, - } - ).encode() - req = urllib.request.Request( - "https://oauth2.googleapis.com/token", - data=data, - headers={"Content-Type": "application/x-www-form-urlencoded"}, - ) - with urllib.request.urlopen(req, timeout=30) as resp: - return json.loads(resp.read())["access_token"] - - else: - raise ValueError(f"Unsupported ADC credential type: {cred_type!r}") - - -# ── Provider resolution ─────────────────────────────────────────────────────── - - -def setup_custom_provider( - provider_name: str, - base_url: str, - api_key: str, - api_protocol: str = "openai-completions", - models: list[dict] | None = None, -): - """Configure an openclaw custom provider in ~/.openclaw/openclaw.json. - - This is the generic replacement for per-provider setup functions. - Any OpenAI-compatible or Anthropic-compatible endpoint can be registered. - """ - config_path = Path.home() / ".openclaw" / "openclaw.json" - config_path.parent.mkdir(parents=True, exist_ok=True) - - # Merge with existing config so multiple providers can coexist - existing = {} - if config_path.exists(): - try: - existing = json.loads(config_path.read_text()) - except (json.JSONDecodeError, OSError): - logger.debug("Could not read existing provider config at %s", config_path) - - providers = existing.setdefault("models", {}).setdefault("providers", {}) - providers[provider_name] = { - "baseUrl": base_url, - "api": api_protocol, - "apiKey": api_key, - "models": models or [], - } - - config_path.write_text(json.dumps(existing, indent=2)) - - -def _infer_provider_prefix(model: str) -> str: - """Infer the openclaw provider prefix from a bare model name. - - Resolution order: - 1. The benchflow provider registry — any registered custom provider - (deepseek/glm/qwen-dashscope/...) that claims this bare model id via - its declared model_prefixes. This routes prefix-stripped ids like - "deepseek-v4-flash" to "deepseek" instead of defaulting to anthropic. - 2. The native gemini/gpt heuristics (openclaw handles these directly). - 3. Anthropic as the final fallback. - """ - m = model.lower() - # 1. Registry-driven bare-model routing (registry owns provider knowledge). - try: - from benchflow.agents.providers import find_provider_for_bare_model - - result = find_provider_for_bare_model(model) - if result is not None: - return result[0] - except ImportError: - logger.debug("benchflow.agents.providers unavailable; using name heuristics") - - # 2. Native providers openclaw recognizes without registry config. - if "gemini" in m: - return "google" - if "gpt" in m or m.startswith(("o1", "o3")): - return "openai" - # 3. Default. - return "anthropic" - - -def _setup_provider_from_config( - provider_name: str, cfg: "ProviderConfig" -) -> str | None: - """Write a resolved registry ProviderConfig into openclaw.json. - - Shared by the prefix-based (``_find_and_setup_provider``) and bare-model - (``_setup_bare_custom_provider``) paths so both register a custom provider - through exactly the same logic. Returns ``provider_name`` on success, or - ``None`` if required config (base_url / api key) is missing — callers then - fall through to their next resolution strategy. - - Raises ``KeyError`` if a required ``url_params`` env var is missing, so the - prefix-based caller can preserve its existing "fall through to the - BENCHFLOW_PROVIDER_* env-var path" behavior on that specific failure. - """ - from benchflow.agents.providers import resolve_base_url - - env = dict(os.environ) - base_url = resolve_base_url(cfg, env) # may raise KeyError (missing url_params) - if cfg.auth_type == "adc": - try: - api_key = _get_adc_token() - except Exception: - logger.debug( - "ADC token acquisition failed for %s", provider_name, exc_info=True - ) - return None - elif cfg.auth_type == "none": - api_key = "" - elif cfg.auth_env: - api_key = env.get(cfg.auth_env, "") - if not api_key: - return None - else: - return None - setup_custom_provider( - provider_name, base_url, api_key, cfg.api_protocol, cfg.models - ) - return provider_name - - -def _setup_bare_custom_provider(model: str) -> str | None: - """Configure the custom provider a BARE model id resolves to, if any. - - Companion to ``_infer_provider_prefix`` for the ``session/set_model`` - "No provider" branch: ``_infer_provider_prefix`` only *names* the provider - prefix, but a registered custom provider (deepseek/glm/qwen-dashscope/...) - must also be written into ``~/.openclaw/openclaw.json`` or openclaw gets a - ``deepseek/...`` model id pointing at a provider it was never told about. - - Resolves ``model`` via ``find_provider_for_bare_model`` (registry-owned - ``model_prefixes``) and, when that names a custom provider, registers it via - the shared ``_setup_provider_from_config`` path. Returns the provider name - if setup succeeded, else ``None`` (openclaw-native ids like gemini/gpt and - unknown ids resolve to no registry provider and need no custom config). - """ - try: - from benchflow.agents.providers import find_provider_for_bare_model - - result = find_provider_for_bare_model(model) - if result is not None: - provider_name, cfg = result - try: - return _setup_provider_from_config(provider_name, cfg) - except KeyError: - # Missing url_params env var — provider can't be configured. - logger.debug( - "Bare model %s resolved to %s but its config env vars are " - "unset; leaving provider unconfigured", - model, - provider_name, - ) - return None - except ImportError: - logger.debug("benchflow.agents.providers unavailable; skipping bare setup") - return None - - -def _find_and_setup_provider(model: str) -> str | None: - """If model matches a custom provider, configure it and return the provider name. - - Returns the registered provider name (e.g. "google-vertex", "custom") so the - caller can prefix the model for openclaw, or None if no provider was set up. - - Resolution order: - 1. If benchflow is importable, try find_provider(model) for prefix-based match. - 2. Fall back to BENCHFLOW_PROVIDER_* env vars injected by the SDK. - This handles stripped model names (no prefix) where the SDK already - resolved the provider and passed config via env vars. - """ - # 1. Try benchflow provider registry (prefix-based match) - try: - from benchflow.agents.providers import find_provider - - result = find_provider(model) - if result is not None: - provider_name, cfg = result - try: - return _setup_provider_from_config(provider_name, cfg) - except KeyError: - pass # missing url_params env var; fall through to env var path - except ImportError: - logger.debug("benchflow.agents.providers not available, using env var fallback") - - # 2. Fall back to BENCHFLOW_PROVIDER_* env vars set by the SDK. - # This is the primary path for stripped model names (e.g. "claude-sonnet-4-6" - # from "anthropic-vertex/claude-sonnet-4-6") where the SDK already resolved - # the provider config. - base_url = os.environ.get("BENCHFLOW_PROVIDER_BASE_URL") - api_key = os.environ.get("BENCHFLOW_PROVIDER_API_KEY") - api_protocol = os.environ.get("BENCHFLOW_PROVIDER_PROTOCOL", "openai-completions") - models_json = os.environ.get("BENCHFLOW_PROVIDER_MODELS", "[]") - # If no explicit API key, try ADC (for Vertex AI providers) - if base_url and not api_key: - try: - api_key = _get_adc_token() - except Exception: - logger.debug("ADC token fallback failed", exc_info=True) - if base_url and api_key: - provider_name = model.split("/")[0] if "/" in model else "custom" - try: - models = json.loads(models_json) - except json.JSONDecodeError: - models = [] - setup_custom_provider(provider_name, base_url, api_key, api_protocol, models) - return provider_name - return None - - -def _resolve_bare_model_prefix(model: str) -> str: - """Resolve (and, where possible, configure) the provider for a BARE model id. - - Used by the ``session/set_model`` branch where ``BENCHFLOW_PROVIDER_NAME`` - is absent. Each step falls through on ``None``: - - 1. ``_setup_bare_custom_provider`` — the registry claims the bare id and - its provider-specific config env vars resolve; the provider is written - into openclaw.json. - 2. ``_find_and_setup_provider`` — generic ``BENCHFLOW_PROVIDER_*`` env - fallback: a harness may inject endpoint config without the provider - name. Explicit endpoint config must win over a prefix guess for a - provider openclaw was never configured with (codex P2 on PR #670). - 3. ``_infer_provider_prefix`` — no config available anywhere; name the - prefix without registration (openclaw-native gemini/gpt ids, or a - custom provider whose run cannot work without its key anyway). - """ - return ( - _setup_bare_custom_provider(model) - or _find_and_setup_provider(model) - or _infer_provider_prefix(model) - ) - - -# ── Session parsing ─────────────────────────────────────────────────────────── - - -def find_session_jsonl() -> Path | None: - """Find the most recent openclaw session JSONL file.""" - home = os.environ.get("HOME", os.path.expanduser("~")) - sessions_dir = Path(home) / ".openclaw" / "agents" / "main" / "sessions" - if not sessions_dir.exists(): - return None - - jsonl_files = sorted( - sessions_dir.glob("*.jsonl"), - key=lambda f: f.stat().st_mtime, - reverse=True, - ) - # Skip .lock files - for f in jsonl_files: - if not f.name.endswith(".lock"): - return f - return None - - -def parse_session_jsonl(path: Path, session_id: str) -> list[dict]: - """Parse openclaw session JSONL and convert to ACP session/update events. - - openclaw JSONL format uses {type: "message", message: {role, content}} entries. - Roles: "user", "assistant", "toolResult" - Content blocks: text, tool_use, thinking (in assistant messages) - """ - updates = [] - try: - with open(path) as f: - for line in f: - line = line.strip() - if not line: - continue - try: - entry = json.loads(line) - except json.JSONDecodeError: - continue - - # openclaw format: {type: "message", message: {role, content}} - if entry.get("type") != "message": - continue - - msg = entry.get("message", {}) - role = msg.get("role", "") - content = msg.get("content", []) - - if role == "assistant" and isinstance(content, list): - for block in content: - block_type = block.get("type", "") - - if block_type in ("text",): - updates.append( - { - "jsonrpc": "2.0", - "method": "session/update", - "params": { - "sessionId": session_id, - "update": { - "sessionUpdate": "text_update", - "text": block.get("text", ""), - }, - }, - } - ) - - elif block_type in ("tool_use", "toolCall"): - _input = block.get("input", block.get("arguments", {})) - _title = _input.get( - "command", - _input.get("description", block.get("name", "tool")), - ) - updates.append( - { - "jsonrpc": "2.0", - "method": "session/update", - "params": { - "sessionId": session_id, - "update": { - "sessionUpdate": "tool_call", - "toolCallId": block.get("id", ""), - "kind": block.get("name", "tool"), - "title": _title, - "status": "completed", - "content": [ - { - "type": "content", - "content": { - "type": "text", - "text": json.dumps(_input)[ - :_TOOL_INPUT_TRUNCATE - ], - }, - } - ], - }, - }, - } - ) - - elif block_type == "thinking": - updates.append( - { - "jsonrpc": "2.0", - "method": "session/update", - "params": { - "sessionId": session_id, - "update": { - "sessionUpdate": "agent_thought", - "text": block.get("thinking", ""), - }, - }, - } - ) - - elif role == "toolResult": - # Emit as tool_call_update (status=completed) to update - # the tool_call record created by the tool_use block - tool_id = msg.get("toolCallId", "") - result_text = "" - if isinstance(content, list): - result_text = " ".join( - b.get("text", "") - for b in content - if isinstance(b, dict) and b.get("type") == "text" - ) - elif isinstance(content, str): - result_text = content - - updates.append( - { - "jsonrpc": "2.0", - "method": "session/update", - "params": { - "sessionId": session_id, - "update": { - "sessionUpdate": "tool_call_update", - "toolCallId": tool_id, - "status": "completed", - "content": [ - { - "type": "content", - "content": { - "type": "text", - "text": result_text[ - :_TOOL_RESULT_TRUNCATE - ], - }, - } - ], - }, - }, - } - ) - - except Exception: - logger.debug("Failed to parse session JSONL for trajectory", exc_info=True) - - return updates - - -# ── Main loop ───────────────────────────────────────────────────────────────── - - -def main(): - setup_openai_auth() - setup_gcloud_adc() - session_id = "openclaw-shim" - cwd = "/app" - - while True: - try: - msg = recv() - except EOFError: - break - - method = msg.get("method", "") - req_id = msg.get("id") - params = msg.get("params", {}) - - if method == "initialize": - send( - { - "jsonrpc": "2.0", - "id": req_id, - "result": { - "protocolVersion": 1, - "agentCapabilities": { - "loadSession": False, - "promptCapabilities": {"image": False, "audio": False}, - }, - "agentInfo": {"name": "openclaw", "version": "1.0"}, - }, - } - ) - - elif method == "session/new": - cwd = params.get("cwd", "/app") - setup_workspace(cwd) - session_id = "openclaw-shim" - send( - { - "jsonrpc": "2.0", - "id": req_id, - "result": {"sessionId": session_id}, - } - ) - - elif method == "session/set_model": - model = params.get("modelId", "") - requested_model = model - # A provider-resolution / config-write failure here must NOT crash the - # shim: an unhandled exception exits rc=1, which benchflow sees as the - # ACP transport dying mid-set_model ("Process closed stdout (rc=1)") — - # the observed openclaw-on-docker failure. Catch, surface the real cause - # on the trajectory (agent_thought) and stderr, and still ACK so the run - # proceeds with the model config that exists. - try: - if model: - # The SDK strips provider prefixes before set_model and passes - # the original provider name via BENCHFLOW_PROVIDER_NAME env var. - # - # Openclaw natively supports google-vertex/ and anthropic/ prefixes - # (via the google plugin enabled at startup). Custom providers like - # zai/ and other custom providers need explicit registration via openclaw.json. - provider_name = os.environ.get("BENCHFLOW_PROVIDER_NAME", "") - - # Native Vertex providers — openclaw handles these via google plugin - if provider_name in ("google-vertex", "anthropic-vertex"): - # Reconstruct the full model name openclaw expects - if "/" not in model: - model = f"{provider_name}/{model}" - # Custom providers — register in openclaw.json - elif provider_name: - _provider_name = _find_and_setup_provider(model) - if _provider_name and "/" not in model: - model = f"{_provider_name}/{model}" - # No provider env var — resolve the bare id: registry-specific - # setup, then the generic BENCHFLOW_PROVIDER_* env fallback, - # then prefix heuristics (see _resolve_bare_model_prefix). - elif "/" not in model: - model = f"{_resolve_bare_model_prefix(model)}/{model}" - - subprocess.run( - [ - _OPENCLAW_BIN, - "config", - "set", - "agents.defaults.model", - model, - ], - capture_output=True, - check=True, - timeout=10, - ) - - max_tokens = _max_tokens_value( - requested_model, os.environ.get("BENCHFLOW_MODEL_MAX_TOKENS") - ) - if max_tokens: - subprocess.run( - [ - _OPENCLAW_BIN, - "config", - "set", - _PARAM_MAP["BENCHFLOW_MODEL_MAX_TOKENS"], - max_tokens, - ], - capture_output=True, - check=True, - timeout=10, - ) - # Apply model generation parameters from env vars - for env_key in ( - "BENCHFLOW_MODEL_TEMPERATURE", - "BENCHFLOW_MODEL_TOP_P", - ): - val = os.environ.get(env_key) - if val: - subprocess.run( - [ - _OPENCLAW_BIN, - "config", - "set", - _PARAM_MAP[env_key], - val, - ], - capture_output=True, - check=True, - timeout=10, - ) - except Exception as exc: - diag = ( - f"[openclaw-acp-shim] set_model setup failed, continuing: {exc!r}" - ) - print(diag, file=sys.stderr, flush=True) - # Surface the real cause on the trajectory too: without this, a - # set_model config failure is indistinguishable downstream from a - # genuine provider outage (both read as an opaque suspected_api_error). - send( - { - "jsonrpc": "2.0", - "method": "session/update", - "params": { - "sessionId": session_id, - "update": { - "sessionUpdate": "agent_thought", - "text": diag[:_DIAG_TRUNCATE], - }, - }, - } - ) - - send({"jsonrpc": "2.0", "id": req_id, "result": {}}) - - elif method == "session/prompt": - prompt_parts = params.get("prompt", []) - text = "" - for part in prompt_parts: - if isinstance(part, dict) and part.get("type") == "text": - text += part.get("text", "") - - try: - result = subprocess.run( - [ - _OPENCLAW_BIN, - "agent", - "--local", - "--agent", - "main", - "--json", - "-m", - text, - "--timeout", - "900", - ], - capture_output=True, - text=True, - timeout=920, - env={**os.environ}, - ) - - # Surface stderr as agent thought (for debugging) - if result.stderr and result.stderr.strip(): - send( - { - "jsonrpc": "2.0", - "method": "session/update", - "params": { - "sessionId": session_id, - "update": { - "sessionUpdate": "agent_thought", - "text": f"[openclaw stderr]\n{result.stderr[:_DIAG_TRUNCATE]}", - }, - }, - } - ) - - # Parse openclaw's session JSONL for full trajectory - # Extract session ID from JSON output (may be multi-line) - oc_session_id = None - try: - # openclaw --json output can be multi-line pretty-printed - stdout = result.stdout.strip() - if stdout: - response_data = json.loads(stdout) - oc_session_id = ( - response_data.get("meta", {}) - .get("agentMeta", {}) - .get("sessionId") - ) - except (json.JSONDecodeError, KeyError, TypeError): - # Try finding sessionId in raw output - import re - - m = re.search(r'"sessionId"\s*:\s*"([^"]+)"', result.stdout or "") - if m: - oc_session_id = m.group(1) - - # Find session JSONL: try specific ID first, then most recent - session_jsonl = None - home = os.environ.get("HOME", os.path.expanduser("~")) - sessions_dir = Path(home) / ".openclaw" / "agents" / "main" / "sessions" - - if oc_session_id: - specific = sessions_dir / f"{oc_session_id}.jsonl" - if specific.exists(): - session_jsonl = specific - - if not session_jsonl: - session_jsonl = find_session_jsonl() - - # Fallback: scan directory for most recent JSONL - if not session_jsonl and sessions_dir.exists(): - for jf in sorted( - sessions_dir.glob("*.jsonl"), - key=lambda f: f.stat().st_mtime, - reverse=True, - ): - if jf.name not in ("sessions.json",) and not jf.name.endswith( - ".lock" - ): - session_jsonl = jf - break - - if session_jsonl: - updates = parse_session_jsonl(session_jsonl, session_id) - for update in updates: - send(update) - - # If no JSONL trajectory, fall back to text response - if not session_jsonl: - try: - response = json.loads(result.stdout) - agent_text = response.get("payloads", [{}])[0].get("text", "") - except (json.JSONDecodeError, IndexError, KeyError): - agent_text = ( - result.stdout[:_DIAG_TRUNCATE] if result.stdout else "" - ) - - if agent_text: - send( - { - "jsonrpc": "2.0", - "method": "session/update", - "params": { - "sessionId": session_id, - "update": { - "sessionUpdate": "text_update", - "text": agent_text, - }, - }, - } - ) - - send( - { - "jsonrpc": "2.0", - "id": req_id, - "result": {"stopReason": "end_turn"}, - } - ) - - except subprocess.TimeoutExpired: - send( - { - "jsonrpc": "2.0", - "id": req_id, - "result": {"stopReason": "end_turn"}, - } - ) - except Exception as e: - send( - { - "jsonrpc": "2.0", - "id": req_id, - "error": {"code": -32603, "message": str(e)}, - } - ) - - elif method == "session/cancel": - send({"jsonrpc": "2.0", "id": req_id, "result": {}}) - - elif method == "session/request_permission": - options = params.get("options", []) - option_id = options[0].get("optionId", "default") if options else "default" - send( - { - "jsonrpc": "2.0", - "id": req_id, - "result": { - "outcome": {"outcome": "selected", "optionId": option_id} - }, - } - ) - - else: - if req_id: - send({"jsonrpc": "2.0", "id": req_id, "result": {}}) - - -if __name__ == "__main__": - main() diff --git a/src/benchflow/agents/registry.py b/src/benchflow/agents/registry.py index 127ef423f..77c1dc639 100644 --- a/src/benchflow/agents/registry.py +++ b/src/benchflow/agents/registry.py @@ -37,19 +37,19 @@ container before launch (e.g. ``~/.codex/auth.json``). - ``home_dirs`` Extra dot-dirs under ``$HOME`` to copy to the sandbox user (for dirs not derivable from ``skill_paths`` / - ``credential_files``, e.g. ``.openclaw``). + ``credential_files``). - ``subscription_auth`` ``SubscriptionAuth`` describing host CLI login files (e.g. ``claude login`` credentials) that can stand in for an API key. API keys still take precedence. Look at the existing entries below for worked examples: ``claude-agent-acp`` (subscription auth + env_mapping), ``codex-acp`` -(credential_files), ``openclaw`` (home_dirs + custom shim), ``gemini`` -(multi-file subscription auth). +(credential_files), and ``gemini`` (multi-file subscription auth). """ import base64 import shlex +import threading from dataclasses import dataclass, field from pathlib import Path @@ -63,10 +63,9 @@ def _install_python_script(container_path: str, source: str) -> str: like `SHIMEOF` or `LAUNCHEREOF` inside the Python source can't collide with a heredoc terminator. - Used by pi-acp, openclaw, and harvey-lab-harness — all three ship a Python - launcher/shim baked into install_cmd. Semantics differ intentionally: - pi and openclaw bridge BENCHFLOW_PROVIDER_* env vars to agent-native - config; harvey-lab delegates to Harvey LAB's own model adapters which + Used by bundled Python launchers/shims baked into install_cmd. Semantics + differ intentionally: pi bridges BENCHFLOW_PROVIDER_* env vars to + agent-native config; harvey-lab delegates to its own model adapters which read provider env vars directly. A shared base is not yet justified — divergence is cheap, premature abstraction isn't. """ @@ -127,7 +126,7 @@ def _apt_install(*packages: str) -> str: f"{_BENCHFLOW_BIN_PREFIX}:{_BENCHFLOW_JS_AGENT_PREFIX}/bin:" f"{_BENCHFLOW_NODE_PREFIX}/bin:$PATH" ) -# Node 22.20.0 supports OpenClaw 2026.6.9. Keep their pin pair in sync. +# Shared JavaScript-agent runtime pin. _NODE_INSTALL = ( "export DEBIAN_FRONTEND=noninteractive; " f"BF_NODE_DIR={_BENCHFLOW_NODE_PREFIX}; " @@ -281,9 +280,6 @@ def _js_agent_launch(binary: str, args: str = "") -> str: ) -# Path to the openclaw ACP shim script -_OPENCLAW_SHIM = (Path(__file__).parent / "openclaw_acp_shim.py").read_text() - # Path to the Pi launch wrapper (bridges BENCHFLOW_PROVIDER_* → Pi config) _PI_LAUNCHER = (Path(__file__).parent / "pi_acp_launcher.py").read_text() @@ -470,7 +466,7 @@ class AgentConfig: # Files to write into container before agent launch (e.g. auth.json). home_dirs: list[str] = field(default_factory=list) # Extra dot-dirs under $HOME to copy to sandbox user (for dirs not - # derivable from skill_paths or credential_files, e.g. ".openclaw"). + # derivable from skill_paths or credential_files). acp_model_format: str = "bare" # How the agent expects ACP model IDs in session/set_model or config options: # "bare" — just the model name (e.g. "claude-sonnet-4-6"). @@ -580,26 +576,6 @@ class AgentConfig: # protocol-dependent translation (env vars for Anthropic, # models.json for OpenAI-compatible providers like vLLM). ), - "openclaw": AgentConfig( - name="openclaw", - description="OpenClaw agent via ACP shim — model set at runtime via --model", - skill_paths=["$HOME/.claude/skills", "$WORKSPACE/skills"], - install_cmd=( - f"{_js_agent_install('openclaw', 'openclaw@2026.6.9')} && " - # Configure: auto-approve tools (no model — set at runtime via ACP set_model) - "mkdir -p ~/.openclaw && " - 'echo \'{"version":1,"defaults":{"allow_all":true}}\'' - " > ~/.openclaw/exec-approvals.json && " - # Deploy ACP shim - + _install_python_script( - f"{_BENCHFLOW_BIN_PREFIX}/openclaw-acp-shim", _OPENCLAW_SHIM - ) - ), - launch_cmd=f"{_BENCHFLOW_BIN_PREFIX}/openclaw-acp-shim", - protocol="acp", - requires_env=[], # inferred from --model at runtime - home_dirs=[".openclaw"], - ), "codex-acp": AgentConfig( name="codex-acp", description="OpenAI Codex agent via ACP", @@ -1007,7 +983,7 @@ def get_sandbox_home_dirs() -> set[str]: Derives from three sources across all registered agents: - credential_files: {home}/.foo/... → ".foo" - subscription_auth.files: {home}/.foo/... → ".foo" - - home_dirs: explicit extras (e.g. ".openclaw") + - home_dirs: explicit extra home directories Skill paths are excluded: deploy_skills() now links those paths directly to a shared skills tree instead of relying on sandbox-home copies. @@ -1070,7 +1046,6 @@ def infer_env_key_for_model(model: str) -> str | None: "codex": "codex-acp", "gemini": "gemini", "pi": "pi-acp", - "openclaw": "openclaw", "openhands": "openhands", "oh": "openhands", "harvey-lab": "harvey-lab-harness", @@ -1079,6 +1054,11 @@ def infer_env_key_for_model(model: str) -> str | None: VALID_PROTOCOLS = {"acp", "acpx", "session-factory"} + +_CORE_AGENT_CONFIGS = dict(AGENTS) +_REGISTRY_LOCK = threading.RLock() + + # --------------------------------------------------------------------------- # The ``acpx:`` runtime-key namespace # --------------------------------------------------------------------------- @@ -1115,19 +1095,21 @@ def acpx_runtime_key(canonical_name: str) -> str: def parse_agent_spec(spec: str) -> tuple[str, str]: - """Parse an agent spec like 'acp/claude-agent-acp', 'acpx/claude', or 'claude'. - - Returns (protocol, agent_name) with alias resolution. - Bare names default to 'acp' protocol. - The 'acpx' protocol routes through the acpx CLI (https://acpx.sh/). - """ + """Parse protocol/name and resolve aliases.""" if "/" in spec: protocol, name = spec.split("/", 1) else: protocol, name = "acp", spec + return protocol, AGENT_ALIASES.get(name, name) + - name = AGENT_ALIASES.get(name, name) - return protocol, name +def is_explicit_raw_agent_command(spec: str) -> bool: + """Return whether *spec* explicitly uses supported raw-command syntax.""" + value = spec.strip() + return bool(value) and ( + any(char.isspace() for char in value) + or value.startswith(("/", "./", "../", "~/")) + ) _ACPX_INSTALL = ( @@ -1237,11 +1219,17 @@ def resolve_agent(spec: str) -> AgentConfig: f"Unknown protocol: {protocol!r}. Valid: {', '.join(sorted(VALID_PROTOCOLS))}" ) + from benchflow.agents import remote_manifests + + remote_manifests.autoload_local_manifest_agents() + protocol, name = parse_agent_spec(spec) + # An already-resolved acpx runtime key (e.g. "acpx:claude-agent-acp") # round-trips: parse_agent_spec leaves it whole under the default "acp" # protocol and it lives in AGENTS. See the ACPX_KEY_PREFIX contract. - if protocol == "acp" and name in AGENTS: - return AGENTS[name] + direct = AGENTS.get(name) if protocol == "acp" else None + if direct is not None: + return direct if name not in AGENTS: shorthand = _resolve_namespace_shorthand(name) @@ -1252,16 +1240,15 @@ def resolve_agent(spec: str) -> AgentConfig: # manifest agents from the pinned agents source (data only — their # install/launch strings run in the sandbox, same trust as task # sources) and retry. One-shot per process; local names always win. - from benchflow.agents import remote_manifests - - if remote_manifests.autoload_remote_manifest_agents(): - name = AGENT_ALIASES.get(name, name) - if name in AGENTS: - config = AGENTS[name] - return _acpx_wrap(config) if protocol == "acpx" else config - shorthand = _resolve_namespace_shorthand(name) - if shorthand is not None: - return _acpx_wrap(shorthand) if protocol == "acpx" else shorthand + remote_manifests.autoload_remote_manifest_agents() + registered_name = AGENT_ALIASES.get(name, name) + config = AGENTS.get(registered_name) + if config is not None: + name = registered_name + return _acpx_wrap(config) if protocol == "acpx" else config + shorthand = _resolve_namespace_shorthand(name) + if shorthand is not None: + return _acpx_wrap(shorthand) if protocol == "acpx" else shorthand from difflib import get_close_matches @@ -1278,14 +1265,15 @@ def resolve_agent(spec: str) -> AgentConfig: ) if remote_manifests.last_source_description: plugin_hint += f" ({remote_manifests.last_source_description} consulted)" - close = get_close_matches(name, list(AGENTS.keys()), n=1, cutoff=0.6) + available_agents = list(AGENTS) + close = get_close_matches(name, available_agents, n=1, cutoff=0.6) if close: raise KeyError( f"Unknown agent: {name!r}. Did you mean: {close[0]!r}?{plugin_hint}" ) raise KeyError( f"Unknown agent: {name!r}. Available: " - f"{', '.join(sorted(AGENTS.keys()))}{plugin_hint}" + f"{', '.join(sorted(available_agents))}{plugin_hint}" ) config = AGENTS[name] @@ -1307,12 +1295,22 @@ def resolve_agent_key(spec: str) -> str: spec string. ``resolve_agent`` then round-trips that key back to the wrapped config. - Unknown agents are returned unchanged so callers can still surface their - own diagnostics (raw-command fallback). + Unknown bare IDs fail closed. Explicit raw commands are returned unchanged. + Built-in non-agent modes and unsupported protocols pass through for their + existing downstream validators. """ + if spec in {"oracle", "acp/oracle"}: + return "oracle" + if spec == "task-runtime": + return spec + protocol, _ = parse_agent_spec(spec) + if protocol not in VALID_PROTOCOLS: + return spec try: config = resolve_agent(spec) except KeyError: + if not is_explicit_raw_agent_command(spec): + raise # The raw-command fallback bypasses resolve_agent's error entirely (the # spec is later exec'd verbatim in the sandbox), so surface the failed- # plugin breadcrumb HERE too — otherwise a plugin load failure manifests @@ -1417,27 +1415,13 @@ def register_agent( disallow_web_tools_owned_paths=disallow_web_tools_owned_paths or [], disallow_web_tools_launch_suffix=disallow_web_tools_launch_suffix, ) - AGENTS[name] = config - AGENT_INSTALLERS[name] = install_cmd - AGENT_LAUNCH[name] = launch_cmd + with _REGISTRY_LOCK: + AGENTS[name] = config + AGENT_INSTALLERS[name] = install_cmd + AGENT_LAUNCH[name] = launch_cmd return config -# --- Opt-in dual-source registry (agent-decoupling decision #7) --------------- -# Merge agents declared as /manifest.toml files under $BENCHFLOW_AGENTS_DIR -# into the registry. A NO-OP when the env var is unset, so a default import of -# core is byte-for-byte unchanged; the manifest path only activates on explicit -# opt-in. The import is deferred to here (end of module) on purpose: manifest.py -# imports AgentConfig from this module, so a top-level import would be circular, -# and the merge must run after AGENTS / AGENT_ALIASES / AGENT_INSTALLERS / -# AGENT_LAUNCH are fully built above. -from benchflow.agents.manifest import ( # noqa: E402 - register_env_manifest_agents as _register_env_manifest_agents, -) - -_register_env_manifest_agents() - - # --- Agent plugin packages (entry-point autoload) ------------------------------ # Out-of-core agent packages (e.g. the benchflow-ai/agents packages) register # their agents either as an import side effect or via a zero-arg ``register()``. diff --git a/src/benchflow/agents/remote_manifests.py b/src/benchflow/agents/remote_manifests.py index 3335137b1..65e20240c 100644 --- a/src/benchflow/agents/remote_manifests.py +++ b/src/benchflow/agents/remote_manifests.py @@ -1,171 +1,265 @@ -"""Miss-driven auto-load of DECLARATIVE agent manifests from a remote source. - -Design: #876 (Phase 2a). When ``--agent `` does not resolve locally, -benchflow fetches the pinned agents source (default: the first-party -``benchflow-ai/agents`` repo, cloned+cached through the same -``benchmark_repos`` machinery task sources use) and registers every -``manifest.toml`` agent found there that does not collide with anything already -registered — then resolution is retried. - -Why this is safe to do automatically, unlike installing agent packages: a -manifest is **pure data**. Its ``install_cmd``/``launch_cmd`` strings execute -inside the task sandbox — exactly the trust level of a task fetched with -``--source-repo`` (and of harbor's ``acp:`` registry auto-fetch, which also -fetches data and executes it sandboxed). No remote code ever runs in the host -process. Host-side *python* agent adapters (e.g. omnigent's session-factory) -are deliberately NOT auto-loaded — those remain explicit installs. - -Semantics: - -* **Gap-fill only** — an agent name or alias that already exists locally - always wins; the remote manifest for it is skipped (never overwritten). -* **One-shot per process** — the first resolution miss triggers at most one - fetch; later misses fail fast as before. -* **Guarded** — a broken manifest (or an unreachable source) logs a warning - and never breaks agent resolution. -* **Opt-out / re-point** — ``BENCHFLOW_AGENTS_SOURCE=off`` disables; - ``BENCHFLOW_AGENTS_SOURCE=owner/repo[@ref]`` re-pins; a local directory path - is also accepted (dev/tests). -""" +"""One-shot loading of declarative agent manifests.""" from __future__ import annotations -import logging import os +import re +import threading +from dataclasses import dataclass, replace from pathlib import Path from benchflow.agents.manifest import ( + AgentManifestError, LoadedManifest, + ManifestIssueKind, + _merge_core_shim_only, discover_manifests, load_agent_manifest, register_manifest_agents, + select_manifest_agents, ) -logger = logging.getLogger(__name__) - +AGENTS_DIR_ENV = "BENCHFLOW_AGENTS_DIR" AGENTS_SOURCE_ENV = "BENCHFLOW_AGENTS_SOURCE" DEFAULT_AGENTS_SOURCE = "benchflow-ai/agents@main" _OFF_VALUES = frozenset({"off", "0", "none", "disabled", "false"}) -# One-shot latch: the first resolution miss triggers at most one fetch per -# process. A human-readable description of what was consulted is kept for the -# unknown-agent error path. -_attempted = False -last_source_description: str = "" +@dataclass(frozen=True) +class ManifestIssue: + kind: ManifestIssueKind + detail: str + path: str = "" + cause: str = "" -def _source_root(spec: str) -> Path: - """Resolve the source spec to a local directory of manifests. + def warning(self) -> str: + prefix = f"{self.path}: " if self.path else "" + return f"{prefix}{self.kind.value}: {self.detail}" - A local directory path is used verbatim (dev/tests); otherwise the spec is - ``owner/repo[@ref]`` and is cloned+cached via the task-source machinery - (data-only shallow clone, same cache as ``--source-repo``). - """ - local = Path(spec).expanduser() - if local.is_dir(): - return local - repo, _, ref = spec.partition("@") - from benchflow._utils.benchmark_repos import resolve_source - return resolve_source(repo, ref=ref or None) +@dataclass(frozen=True) +class ManifestCatalog: + manifests: tuple[LoadedManifest, ...] + issues: tuple[ManifestIssue, ...] + source: str + ref: str + applied: bool = False + @property + def warnings(self) -> tuple[str, ...]: + return tuple(issue.warning() for issue in self.issues) -def _gap_fill( - manifests: list[LoadedManifest], - *, - agents: dict, - aliases: dict, -) -> dict[str, LoadedManifest]: - """Keep only manifests (and aliases) that collide with nothing local. - - Local always wins: an existing agent name, an existing alias, or a name - shadowed by an alias disqualifies the remote manifest; colliding aliases on - an otherwise-fresh manifest are stripped rather than fatal. - """ - kept: dict[str, LoadedManifest] = {} - for lm in manifests: - name = lm.config.name - if name in agents or name in aliases or name in kept: - continue - fresh_aliases = tuple( - a - for a in lm.aliases - if a != name and a not in agents and a not in aliases and a not in kept - ) - kept[name] = LoadedManifest(config=lm.config, aliases=fresh_aliases) - return kept +@dataclass(frozen=True) +class _RequestedSource: + source: str + ref: str + raw_source: str + raw_ref: str + local_root: Path | None = None -def autoload_remote_manifest_agents() -> int: - """Fetch + register remote manifest agents once; return how many were added. - Called from ``resolve_agent``'s miss path. Never raises: any failure logs a - warning and returns 0 so the normal unknown-agent error still surfaces. - """ - global _attempted, last_source_description - if _attempted: - return 0 - _attempted = True +_lock = threading.Lock() +_snapshot: ManifestCatalog | None = None +last_source_description = "" - spec = os.environ.get(AGENTS_SOURCE_ENV, DEFAULT_AGENTS_SOURCE).strip() - if not spec or spec.lower() in _OFF_VALUES: - last_source_description = "agents source disabled" - return 0 - last_source_description = f"agents source {spec!r}" +def _effective_source() -> tuple[str, bool]: + directory = os.environ.get(AGENTS_DIR_ENV, "").strip() + if directory: + return directory, True + return os.environ.get(AGENTS_SOURCE_ENV, DEFAULT_AGENTS_SOURCE).strip(), False + + +def _parse_source(spec: str) -> _RequestedSource: + """Parse source/ref once; strip URL secrets from diagnostics.""" + clean = spec.split("?", 1)[0].split("#", 1)[0] + local = Path(clean).expanduser() + if local.is_dir(): + return _RequestedSource(clean, "", clean, "", local) + source, separator, ref = clean.rpartition("@") + userinfo_only = bool( + separator and re.match(r"^[A-Za-z][A-Za-z0-9+.-]*://[^/]*$", source) + ) + if not separator or not ref or userinfo_only: + source, ref = clean, "" + safe_source = re.sub(r"(://)[^/@]+@", r"\1***@", source) + return _RequestedSource(safe_source, ref, source, ref) + + +def _source_root(request: _RequestedSource) -> Path: + if request.local_root is not None: + return request.local_root + from benchflow._utils.benchmark_repos import resolve_source + + return resolve_source(request.raw_source, ref=request.raw_ref or None) + + +def _read_catalog(spec: str) -> tuple[ManifestCatalog, dict[str, LoadedManifest]]: + requested = _parse_source(spec) + if not spec or spec.lower() in _OFF_VALUES: + issue = ManifestIssue(ManifestIssueKind.DISABLED, "agents source disabled") + return ManifestCatalog((), (issue,), requested.source or "off", ""), {} try: - root = _source_root(spec) + root = _source_root(requested) except Exception as exc: - logger.warning( - "Agent manifest auto-load: could not fetch %s (%s); remote agents " - "unavailable this run.", - spec, - exc, + issue = ManifestIssue( + ManifestIssueKind.UNREACHABLE, + f"catalog unavailable ({type(exc).__name__})", + cause=type(exc).__name__, ) - return 0 + return ManifestCatalog((), (issue,), requested.source, requested.ref), {} - manifests: list[LoadedManifest] = [] + loaded: dict[str, LoadedManifest] = {} + issues: list[ManifestIssue] = [] for path in discover_manifests(root): + rel = path.relative_to(root).as_posix() try: - manifests.append(load_agent_manifest(path)) + manifest = load_agent_manifest(path) + loaded[rel] = manifest + except AgentManifestError as exc: + issues.append( + ManifestIssue( + exc.kind, + f"cannot load manifest ({type(exc).__name__})", + rel, + type(exc).__name__, + ) + ) except Exception as exc: - logger.warning( - "Agent manifest auto-load: skipping unreadable manifest %s: %s", - path, - exc, + issues.append( + ManifestIssue( + ManifestIssueKind.MALFORMED, + f"cannot load manifest ({type(exc).__name__})", + rel, + type(exc).__name__, + ) ) + return ManifestCatalog((), tuple(issues), requested.source, requested.ref), loaded + +def _select( + catalog: ManifestCatalog, + loaded: dict[str, LoadedManifest], + *, + local_override: bool, +) -> tuple[ManifestCatalog, dict[str, LoadedManifest]]: from benchflow.agents.registry import ( + _CORE_AGENT_CONFIGS, AGENT_ALIASES, - AGENT_INSTALLERS, - AGENT_LAUNCH, AGENTS, ) - fresh = _gap_fill(manifests, agents=AGENTS, aliases=AGENT_ALIASES) - if not fresh: - logger.info( - "Agent manifest auto-load: %s had no agents beyond the local registry.", - spec, + eligible = { + manifest.config.name + for manifest in loaded.values() + if local_override + and manifest.config.name in AGENTS + and _CORE_AGENT_CONFIGS.get(manifest.config.name) + == AGENTS[manifest.config.name] + } + agents = {name: config for name, config in AGENTS.items() if name not in eligible} + selected, conflicts = select_manifest_agents( + loaded, agents=agents, aliases=AGENT_ALIASES + ) + collision_issues = tuple( + ManifestIssue(kind, detail, path) + for path, kind, detail in conflicts + if local_override + or kind is not ManifestIssueKind.COLLISION + or loaded[path].config.name not in AGENTS + ) + for name in eligible & selected.keys(): + manifest = selected[name] + selected[name] = LoadedManifest( + _merge_core_shim_only(manifest.config, AGENTS[name]), manifest.aliases ) - return 0 - register_manifest_agents( - fresh, - agents=AGENTS, - aliases=AGENT_ALIASES, - installers=AGENT_INSTALLERS, - launch=AGENT_LAUNCH, + + issues = [*catalog.issues, *collision_issues] + result = replace( + catalog, + manifests=tuple(selected.values()), + issues=tuple( + sorted( + set(issues), + key=lambda issue: (issue.path, issue.kind.value, issue.detail), + ) + ), ) - logger.info( - "Agent manifest auto-load: registered %d agent(s) from %s: %s", - len(fresh), - spec, - ", ".join(sorted(fresh)), + return result, selected + + +def _register_catalog( + catalog: ManifestCatalog, + loaded: dict[str, LoadedManifest], + *, + local_override: bool, +) -> ManifestCatalog: + from benchflow.agents.registry import ( + _REGISTRY_LOCK, + AGENT_ALIASES, + AGENT_INSTALLERS, + AGENT_LAUNCH, + AGENTS, ) - return len(fresh) + + with _REGISTRY_LOCK: + result, selected = _select(catalog, loaded, local_override=local_override) + register_manifest_agents( + selected, + agents=AGENTS, + aliases=AGENT_ALIASES, + installers=AGENT_INSTALLERS, + launch=AGENT_LAUNCH, + override=True, + merge_shim_only=local_override, + ) + return replace(result, applied=True) + + +def ensure_manifest_catalog() -> ManifestCatalog: + """Read, register, then publish one runtime catalog result.""" + global _snapshot, last_source_description + with _lock: + if _snapshot is None: + spec, local_override = _effective_source() + catalog, loaded = _read_catalog(spec) + result = _register_catalog(catalog, loaded, local_override=local_override) + _snapshot = result + last_source_description = ( + "agents source disabled" + if any( + issue.kind is ManifestIssueKind.DISABLED for issue in result.issues + ) + else f"agents source {result.source!r}" + ) + return _snapshot + + +def manifest_catalog_for_listing() -> ManifestCatalog: + """Return applied result, or uncached/non-mutating preview.""" + with _lock: + if _snapshot is not None: + return _snapshot + spec, local_override = _effective_source() + catalog, loaded = _read_catalog(spec) + return _select(catalog, loaded, local_override=local_override)[0] + + +def autoload_remote_manifest_agents() -> int: + """Compatibility wrapper for miss-driven callers.""" + return len(ensure_manifest_catalog().manifests) + + +def autoload_local_manifest_agents() -> None: + """Apply an explicitly configured local catalog before registry lookup.""" + if os.environ.get(AGENTS_DIR_ENV, "").strip(): + ensure_manifest_catalog() def _reset_for_tests() -> None: - global _attempted, last_source_description - _attempted = False - last_source_description = "" + """Clear catalog cache only; fixtures own registry restoration.""" + global _snapshot, last_source_description + with _lock: + _snapshot = None + last_source_description = "" diff --git a/src/benchflow/cli/agent.py b/src/benchflow/cli/agent.py index 3c8be9457..c79a116e8 100644 --- a/src/benchflow/cli/agent.py +++ b/src/benchflow/cli/agent.py @@ -38,12 +38,24 @@ def register_agent(app: typer.Typer) -> None: def agent_list() -> None: """List all registered agents.""" from benchflow.agents.registry import AGENT_ALIASES, list_agents + from benchflow.agents.remote_manifests import ( + manifest_catalog_for_listing, + ) + + catalog = manifest_catalog_for_listing() # Build reverse map: canonical name -> list of aliases - reverse_aliases: dict[str, list[str]] = {} + reverse_aliases: dict[str, set[str]] = {} for alias, canonical in AGENT_ALIASES.items(): if alias != canonical: - reverse_aliases.setdefault(canonical, []).append(alias) + reverse_aliases.setdefault(canonical, set()).add(alias) + if not catalog.applied: + for manifest in catalog.manifests: + for alias in manifest.aliases: + if alias != manifest.config.name: + reverse_aliases.setdefault(manifest.config.name, set()).add( + alias + ) table = Table(title="Registered Agents") table.add_column("Name", style="cyan") @@ -52,7 +64,10 @@ def agent_list() -> None: table.add_column("Protocol", style="green") table.add_column("Requires", style="yellow") - for a in list_agents(): + agents = list_agents() + if not catalog.applied: + agents.extend(manifest.config for manifest in catalog.manifests) + for a in sorted({a.name: a for a in agents}.values(), key=lambda a: a.name): aliases = ", ".join(sorted(reverse_aliases.get(a.name, []))) table.add_row( a.name, aliases, a.description, a.protocol, _format_requires(a) @@ -60,19 +75,27 @@ def agent_list() -> None: console.print(table) console.print(f"[dim]{_REQUIRES_AUTH_NOTE}[/dim]") + if catalog.warnings: + print_error( + f"Agent catalog incomplete ({catalog.source}):\n " + + "\n ".join(catalog.warnings) + ) @agent_app.command("show") def agent_show( name: Annotated[str, typer.Argument(help="Agent name")], ) -> None: """Show details for a registered agent.""" - from benchflow.agents.registry import AGENT_ALIASES, AGENTS + from benchflow.agents.registry import ( + AGENT_ALIASES, + resolve_agent, + ) - resolved = AGENT_ALIASES.get(name, name) - cfg = AGENTS.get(resolved) - if not cfg: - print_error(f"Unknown agent: {name}") - raise typer.Exit(1) + try: + cfg = resolve_agent(name) + except KeyError as exc: + print_error(str(exc)) + raise typer.Exit(1) from None # Collect aliases that point to this agent aliases = sorted( diff --git a/src/benchflow/evaluation.py b/src/benchflow/evaluation.py index 1c96c1f41..4b76dbf33 100644 --- a/src/benchflow/evaluation.py +++ b/src/benchflow/evaluation.py @@ -536,6 +536,8 @@ def __post_init__(self): ) from benchflow.agents.registry import AGENTS + # normalize_agent_name delegates to registry.resolve_agent_key. Unknown + # bare IDs fail before sandbox work; explicit commands remain supported. self.agent = normalize_agent_name(self.agent) self.reasoning_effort = normalize_reasoning_effort(self.reasoning_effort) self.sandbox_user = normalize_sandbox_user(self.sandbox_user) diff --git a/src/benchflow/models.py b/src/benchflow/models.py index b3558e0c5..9a3351383 100644 --- a/src/benchflow/models.py +++ b/src/benchflow/models.py @@ -70,7 +70,7 @@ class RolloutResult: None if verification was skipped or failed. trajectory: Ordered list of ACP session-update dicts (tool calls, messages, thoughts) captured during execution. - agent: Harness name from the registry (e.g. "openclaw"). + agent: Harness name from the registry (e.g. "codex-acp"). agent_name: Name reported by the agent via ACP initialize handshake. model: Model ID used (e.g. "google/gemini-3.1-flash-lite-preview"). n_tool_calls: Total tool calls observed during the session. diff --git a/src/benchflow/providers/litellm_runtime.py b/src/benchflow/providers/litellm_runtime.py index 60c9953c3..65dcfca16 100644 --- a/src/benchflow/providers/litellm_runtime.py +++ b/src/benchflow/providers/litellm_runtime.py @@ -1347,39 +1347,52 @@ def _provider_models_for_proxy_alias( raw: str | None, route: LiteLLMRoute, ) -> str | None: - """Mirror model metadata onto the LiteLLM alias Pi sees in proxy mode. - - Pi resolves ``maxTokens``/``contextWindow`` by looking up the model it is - told to use (the LiteLLM alias) in ``BENCHFLOW_PROVIDER_MODELS``. Without an - alias entry that metadata is lost once traffic is routed through the proxy, - so clone the source entry under the alias id/name. - """ - if not raw: - return None + """Preserve provider metadata when agents see a proxy alias instead of its ID.""" try: - entries = json.loads(raw) + entries = json.loads(raw) if raw else [] except json.JSONDecodeError: return None if not isinstance(entries, list): return None + if any(_provider_model_id(entry) == route.model_alias for entry in entries): + return raw wanted = { route.requested_model, strip_provider_prefix(route.requested_model), route.upstream_model, strip_provider_prefix(route.upstream_model), } - for entry in entries: - entry_id = _provider_model_id(entry) - if entry_id not in wanted: - continue - alias_entry = dict(cast("Mapping[str, Any]", entry)) - alias_entry["id"] = route.model_alias - alias_entry["name"] = route.model_alias - merged = list(entries) - if not any(_provider_model_id(item) == route.model_alias for item in merged): - merged.append(alias_entry) - return json.dumps(merged) - return None + source = next( + (entry for entry in entries if _provider_model_id(entry) in wanted), None + ) + if source is None: + from litellm import get_model_info + + try: + info = get_model_info(route.upstream_model) + except Exception as exc: + # LiteLLM raises plain Exception for unmapped models. Metadata is + # optional; retain existing behavior when its catalog cannot resolve. + logger.debug("Proxy model metadata unavailable: %s", type(exc).__name__) + return None + source = {} + if "reasoning_effort" in (info.get("supported_openai_params") or []): + source["compat"] = {"supportsReasoningEffort": True} + if isinstance(info.get("supports_reasoning"), bool): + source["reasoning"] = info["supports_reasoning"] + for target, key in ( + ("maxTokens", "max_output_tokens"), + ("contextWindow", "max_input_tokens"), + ): + value = info.get(key) + if type(value) is int and value > 0: + source[target] = value + if not source: + return None + source["input"] = ["text", "image"] if info.get("supports_vision") else ["text"] + alias_entry = dict(source) + alias_entry.update(id=route.model_alias, name=route.model_alias) + return json.dumps([*entries, alias_entry]) # Caller-supplied provider endpoints. If any of these survive in the agent env, @@ -1494,6 +1507,11 @@ def _wire_litellm_agent_env( LITELLM_MASTER_KEY_ENV: master_key, } ) + alias_models = _provider_models_for_proxy_alias( + raw=agent_env.get("BENCHFLOW_PROVIDER_MODELS"), route=route + ) + if alias_models: + updated["BENCHFLOW_PROVIDER_MODELS"] = alias_models # Generic model-via-env: an agent whose registration maps # BENCHFLOW_PROVIDER_MODEL into an agent-native env var AND declares # supports_acp_set_model=False states, in data, that launch/env config owns @@ -1571,12 +1589,6 @@ def _wire_litellm_agent_env( updated["BENCHFLOW_PROVIDER_API_KEY"] = master_key updated["BENCHFLOW_PROVIDER_MODEL"] = route.model_alias updated["BENCHFLOW_PROVIDER_NAME"] = "litellm" - alias_models = _provider_models_for_proxy_alias( - raw=agent_env.get("BENCHFLOW_PROVIDER_MODELS"), - route=route, - ) - if alias_models: - updated["BENCHFLOW_PROVIDER_MODELS"] = alias_models return updated agent_cfg = AGENTS.get(agent) diff --git a/src/benchflow/rollout/__init__.py b/src/benchflow/rollout/__init__.py index f7f6695ca..258e544e0 100644 --- a/src/benchflow/rollout/__init__.py +++ b/src/benchflow/rollout/__init__.py @@ -949,9 +949,10 @@ async def setup(self) -> None: self._task.config, cfg.config_override ) - self._disallow_web_tools = ( + has_agent_launch = cfg.primary_agent not in {"oracle", "task-runtime"} + self._disallow_web_tools = has_agent_launch and ( _task_disallows_internet(self._task) or cfg.self_gen_no_internet - ) and cfg.primary_agent != "oracle" + ) self._agent_env = _apply_web_policy( self._planes.resolve_agent_env( cfg.primary_agent, cfg.primary_model, cfg.agent_env @@ -970,10 +971,11 @@ async def setup(self) -> None: _resolve_prompts(cfg.task_path, cfg.prompts), self._task.config.agent.prompt_prefix, ) - self._agent_launch = self._planes.agent_launch( - cfg.primary_agent, - disallow_web_tools=self._disallow_web_tools, - ) + if has_agent_launch: + self._agent_launch = self._planes.agent_launch( + cfg.primary_agent, + disallow_web_tools=self._disallow_web_tools, + ) # Copy task dir to temp when Dockerfile mutations are needed # (_inject_skills writes into environment/_deps/, stage_dockerfile diff --git a/src/benchflow/rollout_planes.py b/src/benchflow/rollout_planes.py index 6d2272d9a..263819861 100644 --- a/src/benchflow/rollout_planes.py +++ b/src/benchflow/rollout_planes.py @@ -23,7 +23,11 @@ deploy_skills, install_agent, ) -from benchflow.agents.registry import AGENT_LAUNCH, AGENTS +from benchflow.agents.registry import ( + AGENT_LAUNCH, + AGENTS, + is_explicit_raw_agent_command, +) from benchflow.environment.manifest import EnvironmentManifest from benchflow.environment.manifest_env import ManifestEnvironment from benchflow.providers.runtime import ( @@ -54,7 +58,11 @@ class DefaultRolloutPlanes: """Default bindings for the four concrete planes.""" def agent_launch(self, agent: str, *, disallow_web_tools: bool) -> str: - launch = AGENT_LAUNCH.get(agent, agent) + launch = AGENT_LAUNCH.get(agent) + if launch is None: + if not is_explicit_raw_agent_command(agent): + raise KeyError(f"Unknown agent: {agent!r}") + launch = agent if not disallow_web_tools: return launch agent_cfg = AGENTS.get(agent) diff --git a/src/benchflow/runtime.py b/src/benchflow/runtime.py index 1c24c8320..76ef9cc0b 100644 --- a/src/benchflow/runtime.py +++ b/src/benchflow/runtime.py @@ -23,7 +23,11 @@ from pathlib import Path from typing import TYPE_CHECKING, Any -from benchflow.agents.registry import AgentConfig, resolve_agent +from benchflow.agents.registry import ( + AgentConfig, + is_explicit_raw_agent_command, + resolve_agent, +) from benchflow.skill_policy import SKILL_MODE_NO_SKILL if TYPE_CHECKING: @@ -194,7 +198,9 @@ def config(self) -> AgentConfig | None: def launch_cmd(self) -> str: config = self.config if config is None: - return self.name + if is_explicit_raw_agent_command(self.name): + return self.name + raise KeyError(f"Unknown agent: {self.name!r}") return config.launch_cmd def __repr__(self) -> str: diff --git a/tests/agents/test_manifest_dirscan.py b/tests/agents/test_manifest_dirscan.py index 8134df5a3..31893375e 100644 --- a/tests/agents/test_manifest_dirscan.py +++ b/tests/agents/test_manifest_dirscan.py @@ -1,13 +1,7 @@ """Directory-scan registration: turn a tree of manifest.toml files into AGENTS -entries (design decision #7, the eve-style filesystem discovery). - -The env-gated entry point ``register_env_manifest_agents`` is a no-op when -``BENCHFLOW_AGENTS_DIR`` is unset — so importing core is unchanged by default; -the dual-source registry only lights up when a developer opts in. Registration -is fail-loud on collision (an agent or alias that already exists is an ambiguous -source of truth, not a silent shadow) unless ``override=True``, and writes every -name-keyed registry map (AGENTS / AGENT_INSTALLERS / AGENT_LAUNCH / AGENT_ALIASES) -the install + rollout paths read from. +entries (design decision #7, the eve-style filesystem discovery). Explicit-map +registration remains fail-loud on collisions and writes every name-keyed map; +environment source selection belongs to ``remote_manifests``. """ from __future__ import annotations @@ -20,7 +14,6 @@ AgentManifestError, discover_manifests, load_agents_from_dir, - register_env_manifest_agents, register_manifest_agents, ) from benchflow.agents.registry import AgentConfig @@ -118,28 +111,6 @@ def test_register_fails_loud_on_alias_collision(tmp_path: Path): register_manifest_agents(load_agents_from_dir(tmp_path), **m) -def test_env_entry_is_noop_when_unset(tmp_path: Path, monkeypatch): - monkeypatch.delenv("BENCHFLOW_AGENTS_DIR", raising=False) - m = _maps() - registered = register_env_manifest_agents(**m) - assert registered == [] - # importing core stays behavior-preserving: nothing touched. - assert all(d == {} for d in m.values()) - - -def test_env_entry_registers_when_set(tmp_path: Path, monkeypatch): - _put(tmp_path, "demo", "demo", extra='aliases = ["demo-code"]\n') - _put(tmp_path, "beta", "beta") - monkeypatch.setenv("BENCHFLOW_AGENTS_DIR", str(tmp_path)) - m = _maps() - registered = register_env_manifest_agents(**m) - assert registered == ["beta", "demo"] - assert set(m["agents"]) == {"beta", "demo"} - assert set(m["installers"]) == {"beta", "demo"} - assert set(m["launch"]) == {"beta", "demo"} - assert m["aliases"] == {"demo-code": "demo"} - - def test_merge_shim_only_keeps_core_shim_fields(tmp_path: Path): # Additive/compatible: a manifest reproducing an existing core agent overrides # it, taking DATA fields from the manifest but the host-side _SHIM_ONLY fields diff --git a/tests/agents/test_manifest_parity.py b/tests/agents/test_manifest_parity.py index e5a45bb24..48c0ed820 100644 --- a/tests/agents/test_manifest_parity.py +++ b/tests/agents/test_manifest_parity.py @@ -9,14 +9,9 @@ Why a child interpreter for "pure core" --------------------------------------- -``registry.py`` calls ``register_env_manifest_agents()`` at import time, so when -``$BENCHFLOW_AGENTS_DIR`` is set (as the dedicated CI job sets it) the in-process -``AGENTS`` is *already* merged with those manifests. Comparing a manifest against -that merged ``AGENTS`` would compare it against *itself* — a silent false pass -that no drift could ever fail. So we recover the un-merged, hand-authored core -configs by re-importing ``registry`` in a child interpreter with -``$BENCHFLOW_AGENTS_DIR`` *unset* (the proven parity recipe), and compare the -live manifests against that. +The parity gate isolates core configs from process-local plugin and catalog +state. It re-imports ``registry`` with ``$BENCHFLOW_AGENTS_DIR`` unset, then +compares live manifests against that clean snapshot. What "byte-identical" means here -------------------------------- @@ -55,7 +50,6 @@ import benchflow from benchflow.agents.manifest import ( - MANIFEST_DIR_ENV, LoadedManifest, _merge_core_shim_only, load_agents_from_dir, @@ -72,6 +66,8 @@ # test_manifest_wiring.py. _SRC = Path(benchflow.__file__).resolve().parents[1] _ROOT = _SRC.parent +MANIFEST_DIR_ENV = "BENCHFLOW_AGENTS_DIR" +MANIFEST_SOURCE_ENV = "BENCHFLOW_AGENTS_SOURCE" _DUMP_PURE_CORE = ( "import json, dataclasses;" @@ -90,8 +86,8 @@ def _pure_core_agents() -> dict[str, AgentConfig]: """``registry.AGENTS`` as authored in core, with the manifest plane gated off. Re-imports ``registry`` in a child interpreter with ``$BENCHFLOW_AGENTS_DIR`` - unset so the import-time manifest merge is a no-op, then ships the configs - back as JSON (``dataclasses.asdict``) and rebuilds them. The ``_SHIM_ONLY`` + unset, then ships configs back as JSON (``dataclasses.asdict``) and rebuilds + them. The ``_SHIM_ONLY`` fields (credential_files / subscription_auth) round-trip as plain dicts, but they are taken from this same core entry during the merge, so they compare equal by construction — only the data fields are meaningfully compared. @@ -119,12 +115,41 @@ def _live_manifests() -> dict[str, LoadedManifest]: return load_agents_from_dir(root) +def _paired_catalog_root() -> str: + """Return configured local catalog, preferring the directory override.""" + raw_dir = os.environ.get(MANIFEST_DIR_ENV, "") + raw_source = os.environ.get(MANIFEST_SOURCE_ENV, "") + source = raw_dir.strip() or raw_source.strip() + if not source: + pytest.skip("paired agents catalog is not configured") + expanded = os.path.expanduser(source) + if not os.path.isdir(expanded): + pytest.skip("paired agents catalog source is not a local directory") + return expanded + + +def test_paired_catalog_has_generic_manifest_shape() -> None: + """Guards PR #1090's local paired-catalog ingestion contract.""" + loaded = load_agents_from_dir(_paired_catalog_root()) + assert loaded + + for name, manifest in loaded.items(): + assert name == manifest.config.name + assert name.strip() + assert manifest.config.install_cmd.strip() + assert manifest.config.launch_cmd.strip() + assert isinstance(manifest.aliases, tuple) + assert len(manifest.aliases) == len(set(manifest.aliases)) + for alias in manifest.aliases: + assert alias.strip() + + def _parity_param_names() -> list[str]: """Core agents that also have a manifest — the set to check for parity. Evaluated at collection time. Empty when ``$BENCHFLOW_AGENTS_DIR`` is unset (no child interpreter is spawned in that case).""" - if not os.environ.get(MANIFEST_DIR_ENV): + if not os.environ.get(MANIFEST_DIR_ENV, "").strip(): return [] core = _pure_core_agents() loaded = _live_manifests() @@ -167,7 +192,9 @@ def test_manifest_byte_identical_to_core(agent_name: str | None) -> None: ) -@pytest.mark.skipif(not os.environ.get(MANIFEST_DIR_ENV), reason=_SKIP_REASON) +@pytest.mark.skipif( + not os.environ.get(MANIFEST_DIR_ENV, "").strip(), reason=_SKIP_REASON +) def test_omnigent_pi_is_sole_core_unmanifested_agent() -> None: """omnigent-pi is the ONLY core agent without a manifest. diff --git a/tests/agents/test_manifest_wiring.py b/tests/agents/test_manifest_wiring.py index 7caaf3881..424002549 100644 --- a/tests/agents/test_manifest_wiring.py +++ b/tests/agents/test_manifest_wiring.py @@ -1,11 +1,4 @@ -"""Import-time activation of the dual-source registry (decision #7 go-live). - -registry.py calls register_env_manifest_agents() at end-of-module. These tests -spawn a *fresh* interpreter so the import-time merge runs cleanly (an in-process -reload would mutate the shared registry the rest of the suite uses). They pin the -gated-off guarantee — a default import is unchanged — and the opt-in behaviour — -$BENCHFLOW_AGENTS_DIR merges a manifest agent into every name-keyed map. -""" +"""Lazy local catalog activation through one ingestion entrypoint.""" from __future__ import annotations @@ -17,11 +10,8 @@ import benchflow -# Resolve absolute import roots so the spawned interpreter finds benchflow -# whether it is installed (CI) or on PYTHONPATH from a worktree (dev VM). _SRC = Path(benchflow.__file__).resolve().parents[1] _ROOT = _SRC.parent - _MANIFEST = """contract_version = "1.0" name = "probe-agent" install_cmd = "echo install" @@ -29,48 +19,50 @@ aliases = ["probe-alias"] """ -_PROBE = ( - "import json;" - "from benchflow.agents.registry import (" - "AGENTS, AGENT_INSTALLERS, AGENT_LAUNCH, AGENT_ALIASES);" - "print(json.dumps({" - "'has': 'probe-agent' in AGENTS," - "'installer': AGENT_INSTALLERS.get('probe-agent')," - "'launch': AGENT_LAUNCH.get('probe-agent')," - "'alias': AGENT_ALIASES.get('probe-alias')," - "}))" -) + +def _fixture(tmp_path: Path) -> Path: + target = tmp_path / "probe" + target.mkdir() + (target / "manifest.toml").write_text(_MANIFEST) + return tmp_path -def _probe(manifest_root: Path, *, set_env: bool) -> dict: +def _probe(root: Path, *, activate: bool) -> dict: + action = ( + "from benchflow.agents.registry import resolve_agent;" + "resolve_agent('probe-agent');" + if activate + else "" + ) + code = ( + "import json;" + "from benchflow.agents.registry import (AGENTS, AGENT_INSTALLERS, " + "AGENT_LAUNCH, AGENT_ALIASES);" + + action + + "print(json.dumps({'has':'probe-agent' in AGENTS," + "'installer':AGENT_INSTALLERS.get('probe-agent')," + "'launch':AGENT_LAUNCH.get('probe-agent')," + "'alias':AGENT_ALIASES.get('probe-alias')}))" + ) env = dict(os.environ) env["PYTHONPATH"] = os.pathsep.join([str(_SRC), str(_ROOT)]) - if set_env: - env["BENCHFLOW_AGENTS_DIR"] = str(manifest_root) - else: - env.pop("BENCHFLOW_AGENTS_DIR", None) - out = subprocess.run( - [sys.executable, "-c", _PROBE], + env["BENCHFLOW_AGENTS_DIR"] = str(root) + env["BENCHFLOW_AGENTS_SOURCE"] = "off" + result = subprocess.run( + [sys.executable, "-c", code], env=env, - cwd=str(_ROOT), + cwd=_ROOT, capture_output=True, text=True, timeout=120, ) - assert out.returncode == 0, out.stderr[-2000:] - return json.loads(out.stdout.strip().splitlines()[-1]) + assert result.returncode == 0, result.stderr[-2000:] + return json.loads(result.stdout.strip().splitlines()[-1]) -def _fixture(tmp_path: Path) -> Path: - d = tmp_path / "probe-agent-dir" # dir name deliberately != agent name - d.mkdir() - (d / "manifest.toml").write_text(_MANIFEST) - return tmp_path - - -def test_default_import_is_unchanged_when_env_unset(tmp_path: Path): - result = _probe(_fixture(tmp_path), set_env=False) - assert result == { +def test_directory_env_does_not_mutate_registry_during_import(tmp_path: Path): + """Guards PR #1090 against a second import-time ingestion plane.""" + assert _probe(_fixture(tmp_path), activate=False) == { "has": False, "installer": None, "launch": None, @@ -78,81 +70,11 @@ def test_default_import_is_unchanged_when_env_unset(tmp_path: Path): } -def test_import_merges_manifest_agent_when_env_set(tmp_path: Path): - result = _probe(_fixture(tmp_path), set_env=True) - assert result["has"] is True - assert result["installer"] == "echo install" - assert result["launch"] == "echo launch" - assert result["alias"] == "probe-agent" - - -_FULL_SNAP = ( - "import json;" - "from benchflow.agents.registry import (" - "AGENTS, AGENT_INSTALLERS, AGENT_LAUNCH, AGENT_ALIASES);" - "print(json.dumps({" - "'agents': sorted(AGENTS)," - "'aliases': dict(sorted(AGENT_ALIASES.items()))," - "'installers': dict(sorted(AGENT_INSTALLERS.items()))," - "'launch': dict(sorted(AGENT_LAUNCH.items()))," - "}, sort_keys=True))" -) - - -def _snapshot(manifest_root: Path, *, set_env: bool) -> dict: - """Full canonical snapshot of all four name-keyed registry maps (fresh interp).""" - env = dict(os.environ) - env["PYTHONPATH"] = os.pathsep.join([str(_SRC), str(_ROOT)]) - if set_env: - env["BENCHFLOW_AGENTS_DIR"] = str(manifest_root) - else: - env.pop("BENCHFLOW_AGENTS_DIR", None) - out = subprocess.run( - [sys.executable, "-c", _FULL_SNAP], - env=env, - cwd=str(_ROOT), - capture_output=True, - text=True, - timeout=120, - ) - assert out.returncode == 0, out.stderr[-2000:] - return json.loads(out.stdout.strip().splitlines()[-1]) - - -def _drop_probe(snap: dict) -> dict: - return { - "agents": [a for a in snap["agents"] if a != "probe-agent"], - "aliases": {k: v for k, v in snap["aliases"].items() if k != "probe-alias"}, - "installers": { - k: v for k, v in snap["installers"].items() if k != "probe-agent" - }, - "launch": {k: v for k, v in snap["launch"].items() if k != "probe-agent"}, +def test_directory_env_activates_all_registry_maps_lazily(tmp_path: Path): + """Guards PR #1090 lazy local catalog registration projections.""" + assert _probe(_fixture(tmp_path), activate=True) == { + "has": True, + "installer": "echo install", + "launch": "echo launch", + "alias": "probe-agent", } - - -def test_default_import_byte_identical_full_registry(tmp_path: Path): - """Airtight 'byte-identical when env unset' guarantee (concern #12). - - Strengthens test_default_import_is_unchanged_when_env_unset from asserting only - the probe agent's ABSENCE to asserting FULL-dict equality of all four name-keyed - registry maps: the sole delta the opt-in manifest plane introduces is the probe - agent itself; every pre-existing agent / alias / installer / launch entry is - byte-for-byte unchanged. Catches accidental mutation of an *existing* entry, - which the absence-only assertion cannot. - """ - fixture = _fixture(tmp_path) - unset = _snapshot(tmp_path, set_env=False) - activated = _snapshot(fixture, set_env=True) - - # gated off: zero probe footprint anywhere in the four maps - assert "probe-agent" not in unset["agents"] - assert "probe-agent" not in unset["installers"] - assert "probe-agent" not in unset["launch"] - assert "probe-alias" not in unset["aliases"] - - # opt-in adds EXACTLY the probe agent — nothing else appears - assert set(activated["agents"]) - set(unset["agents"]) == {"probe-agent"} - assert activated["aliases"].get("probe-alias") == "probe-agent" - - # full byte-identical equality of all four maps, modulo the probe agent - assert unset == _drop_probe(activated) diff --git a/tests/agents/test_plugin_entry_points.py b/tests/agents/test_plugin_entry_points.py index 948e94f37..532981aed 100644 --- a/tests/agents/test_plugin_entry_points.py +++ b/tests/agents/test_plugin_entry_points.py @@ -26,13 +26,17 @@ import pytest -from benchflow.agents import registry +from benchflow.agents import registry, remote_manifests @pytest.fixture(autouse=True) def _clean_failed_plugins(monkeypatch): """Isolate FAILED_AGENT_PLUGINS per test (module-global breadcrumb dict).""" monkeypatch.setattr(registry, "FAILED_AGENT_PLUGINS", {}) + monkeypatch.setenv(remote_manifests.AGENTS_SOURCE_ENV, "off") + remote_manifests._reset_for_tests() + yield + remote_manifests._reset_for_tests() def test_callable_entry_point_is_invoked(monkeypatch): diff --git a/tests/agents/test_remote_manifest_autoload.py b/tests/agents/test_remote_manifest_autoload.py index 88c3c2efd..c415425aa 100644 --- a/tests/agents/test_remote_manifest_autoload.py +++ b/tests/agents/test_remote_manifest_autoload.py @@ -9,6 +9,10 @@ from __future__ import annotations +import os +import subprocess +import sys + import pytest from benchflow.agents import registry, remote_manifests @@ -33,18 +37,22 @@ def _write_manifest(root, dirname, name, extra=""): @pytest.fixture() def source_dir(tmp_path, monkeypatch): + monkeypatch.delenv(remote_manifests.AGENTS_DIR_ENV, raising=False) monkeypatch.setenv(remote_manifests.AGENTS_SOURCE_ENV, str(tmp_path)) remote_manifests._reset_for_tests() + maps = ( + registry.AGENTS, + registry.AGENT_ALIASES, + registry.AGENT_INSTALLERS, + registry.AGENT_LAUNCH, + ) + snapshots = tuple(mapping.copy() for mapping in maps) registered: list[str] = [] yield tmp_path, registered remote_manifests._reset_for_tests() - for name in registered: - registry.AGENTS.pop(name, None) - registry.AGENT_INSTALLERS.pop(name, None) - registry.AGENT_LAUNCH.pop(name, None) - for alias, target in list(registry.AGENT_ALIASES.items()): - if target in registered: - registry.AGENT_ALIASES.pop(alias, None) + for mapping, snapshot in zip(maps, snapshots, strict=True): + mapping.clear() + mapping.update(snapshot) def test_unknown_agent_triggers_autoload_and_resolves(source_dir): @@ -55,6 +63,7 @@ def test_unknown_agent_triggers_autoload_and_resolves(source_dir): def test_gap_fill_never_overwrites_local(source_dir): + """Guards PR #1090 against warnings for expected remote core overlap.""" root, registered = source_dir # remote manifest reuses an existing core name with different commands. _write_manifest(root, "mimo", "mimo") @@ -63,6 +72,7 @@ def test_gap_fill_never_overwrites_local(source_dir): before = registry.AGENTS["mimo"] resolve_agent("probe-remote2") # triggers the load assert registry.AGENTS["mimo"] is before # untouched + assert not remote_manifests.ensure_manifest_catalog().warnings def test_colliding_alias_is_stripped_not_fatal(source_dir): @@ -76,6 +86,28 @@ def test_colliding_alias_is_stripped_not_fatal(source_dir): assert registry.AGENT_ALIASES["claude"] == "claude-agent-acp" +@pytest.mark.parametrize( + ("first_alias", "second_alias"), + [("shared-batch-alias", "shared-batch-alias"), ("probe-b", "")], +) +def test_batch_alias_collisions_are_typed_not_raw( + source_dir, first_alias, second_alias +): + """Guards PR #1090 against raw alias collisions during catalog commit.""" + root, registered = source_dir + _write_manifest(root, "a", "probe-a", f'aliases = ["{first_alias}"]\n') + extra = f'aliases = ["{second_alias}"]\n' if second_alias else "" + _write_manifest(root, "b", "probe-b", extra) + registered.extend(("probe-a", "probe-b")) + + assert resolve_agent("probe-a").name == "probe-a" + assert resolve_agent("probe-b").name == "probe-b" + assert any( + issue.kind.value == "collision" + for issue in remote_manifests.ensure_manifest_catalog().issues + ) + + def test_broken_manifest_skipped_others_load(source_dir, caplog): root, registered = source_dir (root / "broken").mkdir() @@ -107,3 +139,131 @@ def counting(spec): with pytest.raises(KeyError): resolve_agent("nope-2") assert len(calls) == 1 + + +def test_acp_namespace_retries_after_catalog_autoload(source_dir): + """Guards PR #1090 generic acp: catalog resolution.""" + root, registered = source_dir + _write_manifest(root, "probe-namespace", "probe-namespace") + registered.append("probe-namespace") + assert resolve_agent("acp:probe-namespace").name == "probe-namespace" + + +def test_directory_override_wins_over_source(source_dir, monkeypatch, tmp_path): + """Guards PR #1090 single catalog-source precedence.""" + source, registered = source_dir + local = tmp_path / "local" + remote = tmp_path / "remote" + _write_manifest(local, "probe-local", "probe-local") + _write_manifest(remote, "probe-source", "probe-source") + monkeypatch.setenv(remote_manifests.AGENTS_DIR_ENV, str(local)) + monkeypatch.setenv(remote_manifests.AGENTS_SOURCE_ENV, str(remote)) + registered.append("probe-local") + + assert resolve_agent("probe-local").name == "probe-local" + assert remote_manifests.ensure_manifest_catalog().source == str(local) + assert "probe-source" not in registry.AGENTS + assert source != local + + +def test_directory_override_activates_before_builtin_lookup(source_dir, monkeypatch): + """Guards PR #1090 local overrides of directly resolvable built-ins.""" + root, _registered = source_dir + monkeypatch.setenv(remote_manifests.AGENTS_DIR_ENV, str(root)) + _write_manifest(root, "mimo", "mimo") + + config = resolve_agent("mimo") + assert (config.install_cmd, config.launch_cmd) == ("true", "true") + + +def test_resolve_then_listing_reuses_applied_catalog(source_dir, monkeypatch): + """Guards PR #1090 against listing colliding with its applied catalog.""" + root, registered = source_dir + _write_manifest(root, "probe-list", "probe-list", 'aliases = ["probe-short"]\n') + registered.append("probe-list") + calls = 0 + real = remote_manifests._source_root + + def counted(request): + nonlocal calls + calls += 1 + return real(request) + + monkeypatch.setattr(remote_manifests, "_source_root", counted) + assert resolve_agent("probe-list").name == "probe-list" + listing = remote_manifests.manifest_catalog_for_listing() + assert [manifest.config.name for manifest in listing.manifests] == ["probe-list"] + assert not any(issue.kind.value == "collision" for issue in listing.issues) + assert calls == 1 + + +def test_failed_registration_is_not_published(source_dir, monkeypatch): + """Guards PR #1090 registry commit preceding snapshot publication.""" + root, registered = source_dir + _write_manifest(root, "probe-retry", "probe-retry") + registered.append("probe-retry") + real = remote_manifests._register_catalog + calls = 0 + + def fail_once(catalog, loaded, *, local_override): + nonlocal calls + calls += 1 + if calls == 1: + raise RuntimeError("injected before commit") + return real(catalog, loaded, local_override=local_override) + + monkeypatch.setattr(remote_manifests, "_register_catalog", fail_once) + with pytest.raises(RuntimeError, match="injected before commit"): + remote_manifests.ensure_manifest_catalog() + assert remote_manifests._snapshot is None + assert resolve_agent("probe-retry").name == "probe-retry" + assert calls == 2 + + +def test_source_diagnostics_are_sanitized(source_dir, monkeypatch): + """Guards PR #1090 against catalog-source secret leakage.""" + monkeypatch.setenv( + remote_manifests.AGENTS_SOURCE_ENV, + "https://user:password@example.invalid/repo@feature/x?token=secret#fragment", + ) + monkeypatch.setattr( + remote_manifests, + "_source_root", + lambda _: (_ for _ in ()).throw(OSError("raw secret cause")), + ) + catalog = remote_manifests.ensure_manifest_catalog() + assert catalog.source == "https://***@example.invalid/repo" + assert catalog.ref == "feature/x" + assert catalog.issues[0].kind.value == "unreachable" + assert catalog.issues[0].cause == "OSError" + diagnostic = " ".join((catalog.source, *catalog.warnings)) + assert not any( + secret in diagnostic + for secret in ("user", "password", "token", "secret", "fragment") + ) + + +def test_cli_listing_keeps_valid_sibling_and_reports_broken_once(tmp_path): + """Guards PR #1090 generic partial-catalog listing.""" + _write_manifest( + tmp_path, "probe-visible", "probe-visible", 'aliases = ["probe-alias"]\n' + ) + broken = tmp_path / "broken" + broken.mkdir() + (broken / "manifest.toml").write_text("not toml [[[\n") + env = os.environ.copy() + env.pop(remote_manifests.AGENTS_DIR_ENV, None) + env[remote_manifests.AGENTS_SOURCE_ENV] = str(tmp_path) + result = subprocess.run( + [sys.executable, "-m", "benchflow.cli.main", "agent", "list"], + capture_output=True, + text=True, + env=env, + timeout=20, + ) + output = result.stdout + result.stderr + assert result.returncode == 0 + assert "probe-visible" in output + assert "probe-alias" in output + assert output.count("Agent catalog incomplete") == 1 + assert output.count("broken/manifest.toml") == 1 diff --git a/tests/test_acp_model_config_dispatch.py b/tests/test_acp_model_config_dispatch.py index bf3f60a4e..cc6117d57 100644 --- a/tests/test_acp_model_config_dispatch.py +++ b/tests/test_acp_model_config_dispatch.py @@ -219,7 +219,9 @@ async def test_effort_without_effort_config_id_fails_closed(tmp_path): """reasoning_effort requested for an agent that declares no effort config option must fail closed rather than silently drop the effort.""" mock_acp = _make_mocks(config_options=[]) - with pytest.raises(RuntimeError, match="does not declare an ACP effort"): + with pytest.raises( + RuntimeError, match="does not declare or advertise an ACP effort" + ): await _connect( mock_acp, agent="test-agent", @@ -232,40 +234,58 @@ async def test_effort_without_effort_config_id_fails_closed(tmp_path): @pytest.mark.asyncio -async def test_env_owned_model_skips_advertised_model_option(tmp_path): - """A manifest-shaped agent (supports_acp_set_model=False + a - BENCHFLOW_PROVIDER_MODEL env mapping) with the via-env flag set must get NO - ACP model configuration — several registry agents (qwen-code, kilo, - dimcode) advertise a ``model`` config option but validate values against - their own catalog and reject the gateway alias with -32603.""" - from benchflow.agents.registry import AGENT_INSTALLERS, AGENT_LAUNCH, AGENTS - from benchflow.agents.registry import AgentConfig as _AC - - AGENTS["env-owned-probe"] = _AC( - name="env-owned-probe", - install_cmd="true", - launch_cmd="true", - supports_acp_set_model=False, - env_mapping={"BENCHFLOW_PROVIDER_MODEL": "OPENAI_MODEL"}, +async def test_env_owned_model_skips_advertised_model_option(tmp_path, monkeypatch): + """Guards PR #1093: env-owned models bypass advertised ACP model options.""" + from benchflow.agents.registry import AGENTS, AgentConfig + + monkeypatch.setitem( + AGENTS, + "env-owned-probe", + AgentConfig( + name="env-owned-probe", + install_cmd="true", + launch_cmd="true", + supports_acp_set_model=False, + env_mapping={"BENCHFLOW_PROVIDER_MODEL": "OPENAI_MODEL"}, + ), ) - try: - opt = MagicMock() - opt.id = "model" - mock_acp = _make_mocks(config_options=[opt]) - await _connect( - mock_acp, - agent="env-owned-probe", - model="deepseek/deepseek-v4-flash", - tmp_path=tmp_path, - agent_env={ - LITELLM_MODEL_VIA_ENV: "1", - LITELLM_MODEL_ALIAS_ENV: "benchflow-deepseek-deepseek-v4-flash", - "OPENAI_MODEL": "benchflow-deepseek-deepseek-v4-flash", - }, - ) - mock_acp.set_config_option.assert_not_awaited() - mock_acp.set_model.assert_not_awaited() - finally: - AGENTS.pop("env-owned-probe", None) - AGENT_INSTALLERS.pop("env-owned-probe", None) - AGENT_LAUNCH.pop("env-owned-probe", None) + mock_acp = _make_mocks(config_options=[{"id": "model"}]) + await _connect( + mock_acp, + agent="env-owned-probe", + model="deepseek/deepseek-v4-flash", + tmp_path=tmp_path, + agent_env={ + LITELLM_MODEL_VIA_ENV: "1", + LITELLM_MODEL_ALIAS_ENV: "benchflow-deepseek-deepseek-v4-flash", + "OPENAI_MODEL": "benchflow-deepseek-deepseek-v4-flash", + }, + ) + mock_acp.set_config_option.assert_not_awaited() + mock_acp.set_model.assert_not_awaited() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("declared", ["", "custom-effort"]) +async def test_effort_advertisement_and_registry_override( + tmp_path, monkeypatch, declared +): + """Guards PR #1093 external effort discovery and registry precedence.""" + from types import SimpleNamespace + + from benchflow.acp import runtime + + monkeypatch.setitem( + runtime.AGENTS, + "external-test", + SimpleNamespace(acp_effort_config_id=declared), + ) + mock_acp = _make_mocks(config_options=[{"id": "effort"}, {"id": "custom-effort"}]) + await _connect( + mock_acp, + agent="external-test", + model=None, + tmp_path=tmp_path, + reasoning_effort="low", + ) + mock_acp.set_config_option.assert_awaited_once_with(declared or "effort", "low") diff --git a/tests/test_agent_model_decouple.py b/tests/test_agent_model_decouple.py index d01936a14..a31338570 100644 --- a/tests/test_agent_model_decouple.py +++ b/tests/test_agent_model_decouple.py @@ -12,15 +12,6 @@ class TestGetAgent: """get_agent resolves agents correctly.""" - def test_openclaw_direct(self): - config, model = get_agent("openclaw") - assert config.name == "openclaw" - assert model == "" - - def test_openclaw_no_hardcoded_requires_env(self): - config, _ = get_agent("openclaw") - assert config.requires_env == [] - def test_unknown_agent_raises(self): with pytest.raises(KeyError, match="Unknown agent"): get_agent("nonexistent-agent") diff --git a/tests/test_agent_spec.py b/tests/test_agent_spec.py index c58a75410..82e890cc3 100644 --- a/tests/test_agent_spec.py +++ b/tests/test_agent_spec.py @@ -7,12 +7,37 @@ AGENT_INSTALLERS, AGENT_LAUNCH, AGENTS, + is_explicit_raw_agent_command, parse_agent_spec, resolve_agent, resolve_agent_key, ) +@pytest.mark.parametrize( + ("spec", "expected"), + [ + ("python agent.py", True), + ("agent --flag", True), + ("/opt/bin/agent", True), + ("./agent", True), + ("../agent", True), + ("~/bin/agent", True), + ("nova", False), + (" acp:nova ", False), + ("acp/nova", False), + ("acpx/nova", False), + (".hidden", False), + ("~user", False), + ("", False), + (" ", False), + ], +) +def test_explicit_raw_agent_command_predicate(spec, expected): + """Guards PR #1090 raw-command boundary shared by launch paths.""" + assert is_explicit_raw_agent_command(spec) is expected + + class TestParseAgentSpec: """Test parse_agent_spec() protocol/name parsing.""" @@ -183,8 +208,28 @@ def test_acpx_key_round_trips_through_resolve_agent(self): assert config.name == key assert "acpx" in config.launch_cmd - def test_unknown_agent_passes_through(self): - assert resolve_agent_key("totally-unknown-agent") == "totally-unknown-agent" + def test_unknown_bare_agent_fails_closed(self): + """Guards PR #1090 against treating misspelled IDs as commands.""" + with pytest.raises(KeyError, match="Unknown agent"): + resolve_agent_key("totally-unknown-agent") + + @pytest.mark.parametrize( + "command", + [ + "python agent.py", + "agent --flag", + "/opt/bin/agent", + "./agent", + "../agent", + "~/bin/agent", + ], + ) + def test_explicit_raw_commands_pass_through(self, command): + """Guards PR #1090 explicit raw-command compatibility.""" + assert resolve_agent_key(command) == command + + def test_oracle_passes_through(self): + assert resolve_agent_key("oracle") == "oracle" def test_idempotent(self): first = resolve_agent_key("acpx/codex") diff --git a/tests/test_bare_model_provider.py b/tests/test_bare_model_provider.py index fe1036f97..74156b3c8 100644 --- a/tests/test_bare_model_provider.py +++ b/tests/test_bare_model_provider.py @@ -1,137 +1,12 @@ -"""BF-4: bare (prefix-stripped) model ids resolve to the right provider. - -Guards the fix from benchflow-ai/benchflow PR #670 (BF-4) against the -regression where bare custom-provider ids defaulted to anthropic. - -``find_provider`` only matches an explicit ``provider/`` prefix, so after -``strip_provider_prefix`` runs, a bare id like ``deepseek-v4-flash`` no longer -resolves and the openclaw shim's ``_infer_provider_prefix`` historically -defaulted everything that was not gemini/gpt to ``anthropic`` — silently -running deepseek/glm/qwen/... as anthropic. - -These tests pin the registry-driven bare-model routing: - - ``find_provider_for_bare_model`` maps a bare id to its provider via each - provider's declared ``model_prefixes`` (registry owns the knowledge). - - ``_infer_provider_prefix`` consults that helper before its native - gemini/gpt heuristics, and still falls back to anthropic. - - ``_setup_bare_custom_provider`` (Codex P1 follow-up on PR #670) actually - registers the resolved custom provider in openclaw.json before the bare id - is prefixed, while openclaw-native and unknown ids trigger no setup. - - ``_resolve_bare_model_prefix`` (Codex P2 follow-up on PR #670) falls back - to the generic ``BENCHFLOW_PROVIDER_*`` env setup when the registry config - is unresolvable, instead of prefixing an unconfigured provider. -""" +"""Bare model IDs resolve through generic provider-registry metadata.""" import pytest -from benchflow.agents.openclaw_acp_shim import ( - _default_max_tokens, - _infer_provider_prefix, - _max_tokens_value, - _resolve_bare_model_prefix, - _setup_bare_custom_provider, -) -from benchflow.agents.providers import ( - find_provider, - find_provider_for_bare_model, -) -from benchflow.providers.litellm_config import safe_model_alias - - -@pytest.mark.parametrize( - ("model", "cap"), - [ - ("gpt-5.4", 128000), - ("openai/gpt-5.4", 128000), - ("us-openai/gpt-5.4", 128000), - ("azure-foundry-openai/gpt-5.4", 128000), - ("claude-sonnet-4-6", 128000), - ("claude-opus-4-6", 128000), - ("claude-opus-4-7", 128000), - ("claude-opus-4-8", 128000), - ("claude-sonnet-5", 128000), - ("claude-opus-5-1", 128000), - ("anthropic/claude-opus-5", 128000), - ("anthropic-vertex/claude-sonnet-5-2", 128000), - ("azure-foundry-anthropic/claude-sonnet-4-6", 128000), - ("claude-sonnet-4-60", None), - ("claude-sonnet-5-beta", None), - ("claude-opus-5-1-2", None), - ("claude-opus-50", None), - ("not-claude-sonnet-5", None), - ("evil/claude-opus-5", None), - ("evil/gpt-5.4", None), - ], -) -def test_model_token_cap(model, cap): - assert _default_max_tokens(model) == cap - assert _default_max_tokens(safe_model_alias(model)) == cap - - -@pytest.mark.parametrize( - ("configured", "value"), - [ - (None, "128000"), - ("invalid", "128000"), - ("-1", "128000"), - ("127999", "127999"), - ("128000", "128000"), - ("128001", "128000"), - ("9" * 5000, "128000"), - ], -) -def test_max_tokens_value(configured, value): - assert _max_tokens_value("claude-sonnet-5", configured) == value - - -def test_uncapped_model_preserves_configured_max_tokens(): - assert _max_tokens_value("other-model", "invalid") == "invalid" - - -def test_set_model_writes_max_tokens_before_optional_params(monkeypatch): - import benchflow.agents.openclaw_acp_shim as shim - - monkeypatch.setattr(shim, "setup_openai_auth", lambda: None) - monkeypatch.setattr(shim, "setup_gcloud_adc", lambda: None) - monkeypatch.setenv("BENCHFLOW_MODEL_MAX_TOKENS", "128001") - monkeypatch.setenv("BENCHFLOW_MODEL_TEMPERATURE", "0.5") - - calls = [] - - def run(args, **_): - calls.append(args) - if args[-2] == "agents.defaults.params.temperature": - raise RuntimeError("temperature config failed") - - monkeypatch.setattr(shim.subprocess, "run", run) - messages = [ - { - "id": 1, - "method": "session/set_model", - "params": {"modelId": "anthropic/claude-sonnet-5"}, - } - ] - - def recv(): - if messages: - return messages.pop() - raise EOFError - - monkeypatch.setattr(shim, "recv", recv) - monkeypatch.setattr(shim, "send", lambda _: None) - - shim.main() - - writes = [(call[-2], call[-1]) for call in calls] - assert writes == [ - ("agents.defaults.model", "anthropic/claude-sonnet-5"), - ("agents.defaults.params.maxTokens", "128000"), - ("agents.defaults.params.temperature", "0.5"), - ] +from benchflow.agents.providers import find_provider, find_provider_for_bare_model class TestFindProviderForBareModel: - """Registry helper: bare model id -> (provider_name, config).""" + """Bare model id maps to provider without shim-owned behavior.""" @pytest.mark.parametrize( ("model", "expected"), @@ -140,7 +15,7 @@ class TestFindProviderForBareModel: ("deepseek-v4-pro", "deepseek"), ("glm-4.6", "glm"), ("glm-5.1", "glm"), - ("qwen3.6-max-preview", "qwen-dashscope"), # version-suffixed, no hyphen + ("qwen3.6-max-preview", "qwen-dashscope"), ("qwen-max", "qwen-dashscope"), ("kimi-k2.6", "kimi"), ("moonshot-v1-8k", "kimi"), @@ -150,288 +25,42 @@ class TestFindProviderForBareModel: ], ) def test_known_families_resolve(self, model, expected): + """Guards PR #670 provider routing after PR B for issue #1090.""" result = find_provider_for_bare_model(model) - assert result is not None, f"{model!r} did not resolve" + assert result is not None assert result[0] == expected def test_case_insensitive(self): assert find_provider_for_bare_model("DeepSeek-V4-Flash")[0] == "deepseek" def test_longest_token_wins_for_doubao(self): - """doubao-seed-2-pro/-lite carry full family tokens; the longer wins.""" - assert ( - find_provider_for_bare_model("doubao-seed-2-pro-251015")[0] - == "doubao-seed-2-pro" + assert find_provider_for_bare_model("doubao-seed-2-pro-251015")[0] == ( + "doubao-seed-2-pro" ) - assert ( - find_provider_for_bare_model("doubao-seed-2-lite-251015")[0] - == "doubao-seed-2-lite" + assert find_provider_for_bare_model("doubao-seed-2-lite-251015")[0] == ( + "doubao-seed-2-lite" ) def test_token_requires_family_boundary(self): - """A different word that merely starts with a token must NOT match.""" assert find_provider_for_bare_model("glmnext-9b") is None assert find_provider_for_bare_model("deepseekish-1b") is None - def test_unknown_model_returns_none(self): - assert find_provider_for_bare_model("whatever-7b") is None - assert find_provider_for_bare_model("claude-sonnet-4-6") is None - assert find_provider_for_bare_model("gpt-4o") is None - assert find_provider_for_bare_model("gemini-3.1-flash-lite") is None - - def test_prefixed_input_defers_to_find_provider(self): - """Inputs still carrying a registered provider/ prefix return None here.""" - assert find_provider_for_bare_model("deepseek/deepseek-v4-flash") is None - assert find_provider_for_bare_model("zai/glm-5") is None - # Sanity: those DO resolve via the prefix-based find_provider. - assert find_provider("deepseek/deepseek-v4-flash")[0] == "deepseek" - - def test_empty_input_returns_none(self): - assert find_provider_for_bare_model("") is None - assert find_provider_for_bare_model(" ") is None - - -class TestInferProviderPrefixRegistry: - """_infer_provider_prefix consults the registry, then heuristics, then anthropic.""" - - @pytest.mark.parametrize( - ("model", "expected"), - [ - # BF-4 fix: bare custom-provider ids route via the registry. - ("deepseek-v4-flash", "deepseek"), - ("glm-4.6", "glm"), - ("qwen3.6-max-preview", "qwen-dashscope"), - ("minimax-m2.7", "minimax"), - # Native heuristics unchanged. - ("gemini-3.1-flash-lite", "google"), - ("gemini-2.5-pro", "google"), - ("gpt-4o", "openai"), - ("o1-preview", "openai"), - ("o3-mini", "openai"), - # Anthropic stays the default for genuinely unknown ids. - ("whatever-7b", "anthropic"), - ("claude-sonnet-4-6", "anthropic"), - ("claude-haiku-4-5-20251001", "anthropic"), - ], - ) - def test_infer(self, model, expected): - assert _infer_provider_prefix(model) == expected - - -class TestSetupBareCustomProvider: - """Codex P1 (PR #670): bare custom ids must REGISTER their provider. - - ``_infer_provider_prefix`` only names the prefix; for a registered custom - provider the shim must also write the provider into ``openclaw.json``, or - openclaw receives a ``deepseek/...`` id pointing at a provider it never - learned about and the run fails. These tests spy on ``setup_custom_provider`` - (the openclaw.json writer) to prove the bare custom path triggers setup and - that openclaw-native / unknown ids do NOT. - """ - - def test_bare_custom_model_registers_provider(self, monkeypatch): - """deepseek-v4-flash → setup_custom_provider called with deepseek config.""" - monkeypatch.setenv("DEEPSEEK_BASE_URL", "https://api.deepseek.test/v1") - monkeypatch.setenv("DEEPSEEK_API_KEY", "sk-deepseek-test") - - calls = [] - monkeypatch.setattr( - "benchflow.agents.openclaw_acp_shim.setup_custom_provider", - lambda *a, **k: calls.append((a, k)), - ) - - provider = _setup_bare_custom_provider("deepseek-v4-flash") - - assert provider == "deepseek" - assert len(calls) == 1 - args, _ = calls[0] - # setup_custom_provider(provider_name, base_url, api_key, api_protocol, models) - assert args[0] == "deepseek" - assert args[1] == "https://api.deepseek.test/v1" - assert args[2] == "sk-deepseek-test" - - def test_bare_custom_model_uses_registry_endpoint(self, monkeypatch): - """glm-4.6 routes to the glm provider with its own env-supplied endpoint.""" - monkeypatch.setenv("GLM_BASE_URL", "https://glm.test/v1") - monkeypatch.setenv("GLM_API_KEY", "glm-test-key") - - calls = [] - monkeypatch.setattr( - "benchflow.agents.openclaw_acp_shim.setup_custom_provider", - lambda *a, **k: calls.append(a), - ) - - assert _setup_bare_custom_provider("glm-4.6") == "glm" - assert len(calls) == 1 - assert calls[0][0] == "glm" - @pytest.mark.parametrize( "model", [ - "gemini-3.1-flash-lite", - "gpt-4o", - "o3-mini", - "claude-sonnet-4-6", + "", + " ", "whatever-7b", + "claude-sonnet-4-6", + "gpt-4o", + "gemini-3.1-flash-lite", ], ) - def test_native_and_unknown_models_do_not_register(self, model, monkeypatch): - """openclaw-native (gemini/gpt/claude) and unknown ids trigger NO setup.""" - calls = [] - monkeypatch.setattr( - "benchflow.agents.openclaw_acp_shim.setup_custom_provider", - lambda *a, **k: calls.append(a), - ) - - assert _setup_bare_custom_provider(model) is None - assert calls == [] - - def test_missing_config_env_does_not_register(self, monkeypatch): - """A resolved custom provider with unset url_params/key registers nothing.""" - # Ensure the deepseek config env vars are absent. - monkeypatch.delenv("DEEPSEEK_BASE_URL", raising=False) - monkeypatch.delenv("DEEPSEEK_API_KEY", raising=False) - - calls = [] - monkeypatch.setattr( - "benchflow.agents.openclaw_acp_shim.setup_custom_provider", - lambda *a, **k: calls.append(a), - ) - - # Resolves to deepseek in the registry, but can't be configured → None. - assert _setup_bare_custom_provider("deepseek-v4-flash") is None - assert calls == [] - - -class TestResolveBareModelPrefix: - """Codex P2 (PR #670): generic env fallback before prefixing bare ids. - - Guards the fix from benchflow-ai/benchflow PR #670 against the regression - where a bare custom id whose registry config could not resolve (e.g. - DEEPSEEK_BASE_URL/DEEPSEEK_API_KEY unset) was prefixed via - ``_infer_provider_prefix`` anyway — handing openclaw ``deepseek/`` - for a provider never written to openclaw.json — even though the generic - ``BENCHFLOW_PROVIDER_BASE_URL``/``BENCHFLOW_PROVIDER_API_KEY`` envs could - have configured a working provider via ``_find_and_setup_provider``. - """ - - GENERIC_ENVS = ( - "BENCHFLOW_PROVIDER_BASE_URL", - "BENCHFLOW_PROVIDER_API_KEY", - "BENCHFLOW_PROVIDER_PROTOCOL", - "BENCHFLOW_PROVIDER_MODELS", - ) - - @pytest.fixture(autouse=True) - def _clean_env(self, monkeypatch): - """Start from no provider config; individual tests opt back in.""" - for var in (*self.GENERIC_ENVS, "DEEPSEEK_BASE_URL", "DEEPSEEK_API_KEY"): - monkeypatch.delenv(var, raising=False) - - @pytest.fixture - def setup_spy(self, monkeypatch): - calls = [] - monkeypatch.setattr( - "benchflow.agents.openclaw_acp_shim.setup_custom_provider", - lambda *a, **k: calls.append(a), - ) - return calls - - def test_generic_envs_used_when_registry_config_unresolvable( - self, monkeypatch, setup_spy - ): - """Registry names deepseek but its envs are unset → generic setup runs.""" - monkeypatch.setenv("BENCHFLOW_PROVIDER_BASE_URL", "https://proxy.test/v1") - monkeypatch.setenv("BENCHFLOW_PROVIDER_API_KEY", "sk-generic-test") - - assert _resolve_bare_model_prefix("deepseek-v4-flash") == "custom" - assert len(setup_spy) == 1 - # setup_custom_provider(provider_name, base_url, api_key, protocol, models) - assert setup_spy[0][:3] == ( - "custom", - "https://proxy.test/v1", - "sk-generic-test", - ) - - def test_registry_config_wins_over_generic_envs(self, monkeypatch, setup_spy): - """Provider-specific envs take precedence over the generic fallback.""" - monkeypatch.setenv("DEEPSEEK_BASE_URL", "https://api.deepseek.test/v1") - monkeypatch.setenv("DEEPSEEK_API_KEY", "sk-deepseek-test") - monkeypatch.setenv("BENCHFLOW_PROVIDER_BASE_URL", "https://proxy.test/v1") - monkeypatch.setenv("BENCHFLOW_PROVIDER_API_KEY", "sk-generic-test") - - assert _resolve_bare_model_prefix("deepseek-v4-flash") == "deepseek" - assert len(setup_spy) == 1 - assert setup_spy[0][0] == "deepseek" - - def test_no_config_anywhere_prefixes_without_registration(self, setup_spy): - """With neither registry nor generic envs, keep the inferred prefix - and register nothing (the run cannot work without a key anyway).""" - assert _resolve_bare_model_prefix("deepseek-v4-flash") == "deepseek" - assert setup_spy == [] - - @pytest.mark.parametrize( - ("model", "expected"), - [ - ("gemini-3.1-flash-lite", "google"), - ("gpt-4o", "openai"), - ("o3-mini", "openai"), - ("claude-sonnet-4-6", "anthropic"), - ("whatever-7b", "anthropic"), - ], - ) - def test_native_and_unknown_ids_unchanged(self, model, expected, setup_spy): - """Without generic envs, builtin/unknown ids resolve exactly as before.""" - assert _resolve_bare_model_prefix(model) == expected - assert setup_spy == [] - - -def test_set_model_failure_acks_and_emits_thought_without_crashing(monkeypatch): - # Contract for PR #871's openclaw set_model swallow: a provider-resolution / - # config-write failure must NOT crash the shim (rc=1). It must still ACK the - # request AND surface the real cause on the trajectory (agent_thought) so the - # failure is not indistinguishable downstream from a genuine provider outage. - import benchflow.agents.openclaw_acp_shim as shim - - monkeypatch.setattr(shim, "setup_openai_auth", lambda: None) - monkeypatch.setattr(shim, "setup_gcloud_adc", lambda: None) + def test_unknown_or_empty_returns_none(self, model): + assert find_provider_for_bare_model(model) is None - def _boom(*args, **kwargs): - raise RuntimeError("config set exploded") - - monkeypatch.setattr(shim.subprocess, "run", _boom) - - inbox = iter( - [ - { - "jsonrpc": "2.0", - "id": 7, - "method": "session/set_model", - "params": {"modelId": "deepseek/deepseek-v4-flash"}, - } - ] - ) - - def _fake_recv(): - try: - return next(inbox) - except StopIteration: - raise EOFError from None - - sent: list = [] - monkeypatch.setattr(shim, "recv", _fake_recv) - monkeypatch.setattr(shim, "send", sent.append) - - shim.main() # must return normally — no rc=1 crash - - acks = [m for m in sent if m.get("id") == 7 and "result" in m] - assert acks == [{"jsonrpc": "2.0", "id": 7, "result": {}}] - assert not any(m.get("id") == 7 and "error" in m for m in sent) - - thoughts = [ - m - for m in sent - if m.get("method") == "session/update" - and m["params"]["update"].get("sessionUpdate") == "agent_thought" - ] - assert any("config set exploded" in t["params"]["update"]["text"] for t in thoughts) + def test_prefixed_input_defers_to_find_provider(self): + for model in ("deepseek/deepseek-v4-flash", "zai/glm-5"): + assert find_provider_for_bare_model(model) is None + assert find_provider("deepseek/deepseek-v4-flash")[0] == "deepseek" + assert find_provider("zai/glm-5")[0] == "zai" diff --git a/tests/test_capture_trajectory.py b/tests/test_capture_trajectory.py index eac6622f6..1f4af6330 100644 --- a/tests/test_capture_trajectory.py +++ b/tests/test_capture_trajectory.py @@ -598,8 +598,8 @@ def test_partial_timeout_preserves_order(self) -> None: assert result[0]["text"] == "Solve it" assert result[2]["status"] == ToolCallStatus.PENDING.value - def test_openclaw_text_update_and_agent_thought(self) -> None: - """openclaw shim uses text_update and agent_thought (not chunks).""" + def test_full_text_update_and_agent_thought(self) -> None: + """Full text_update and agent_thought events are captured.""" session = ACPSession("s1") session.record_user_prompt("Go") session.handle_update( diff --git a/tests/test_integration_matrix.py b/tests/test_integration_matrix.py index 8bc138de6..0a0e615f7 100644 --- a/tests/test_integration_matrix.py +++ b/tests/test_integration_matrix.py @@ -215,13 +215,6 @@ def test_deepseek_tiering_flash_for_low_pro_for_high(): assert low_models == {maps.baseline_model} -def test_agents_rule_acp_shim_path_map(): - maps = _maps() - plan = _plan(["src/benchflow/agents/openclaw_acp_shim.py"]) - assert "openclaw" in _agents(plan) - assert maps.baseline_agent in _agents(plan) - - def test_agent_runtime_infra_fans_roster_subset(): # Changes to the registry / shared ACP infra affect EVERY agent, but at L2 # (auto-on-push) we fan only the representative SUBSET — one agent per @@ -573,7 +566,6 @@ def test_per_agent_concurrency_clamped_for_full_daytona_roster(): "path,agent", [ ("src/benchflow/agents/codex_config.py", "codex-acp"), - ("src/benchflow/agents/openclaw_acp_shim.py", "openclaw"), ("src/benchflow/agents/claude_agent_acp.py", "claude-agent-acp"), ("src/benchflow/agents/pi_acp_launcher.py", "pi-acp"), ], diff --git a/tests/test_job.py b/tests/test_job.py index 447138a69..b3d3a4608 100644 --- a/tests/test_job.py +++ b/tests/test_job.py @@ -452,7 +452,7 @@ async def test_config_mismatch_refuses_scoped(self, tmp_path): 'version = "1.0"\n[verifier]\ntimeout_sec = 60\n' "[agent]\ntimeout_sec = 60\n[environment]\n" ) - cfg = EvaluationConfig(agent="new-agent") + cfg = EvaluationConfig(agent="new-agent --run") job = Evaluation( tasks_dir=tasks_dir, jobs_dir=jobs_dir, config=cfg, job_name="my-job" ) diff --git a/tests/test_litellm_config.py b/tests/test_litellm_config.py index 188c1a2e6..a79ad0db7 100644 --- a/tests/test_litellm_config.py +++ b/tests/test_litellm_config.py @@ -3,6 +3,7 @@ import pytest from benchflow.agents.env import resolve_agent_env, resolve_provider_env +from benchflow.agents.registry import AGENTS, AgentConfig from benchflow.providers.litellm_config import ( litellm_proxy_config, resolve_litellm_route, @@ -132,19 +133,18 @@ def test_registered_provider_route_honors_explicit_generic_proxy_env(): assert route.required_env == ("BENCHFLOW_PROVIDER_API_KEY",) -@pytest.mark.parametrize( - ("agent", "agent_base"), - [ - ("claude-agent-acp", "https://api.z.ai/api/anthropic"), - ("openclaw", "https://api.z.ai/api/coding/paas/v4"), - ], -) -def test_zai_coding_clawsbench_routes(agent, agent_base): - """Guards PR #1074: ClawsBench agents use each supported Z.AI surface.""" +def test_zai_coding_openai_protocol_route(monkeypatch): + """Guards PR #1074: generic agents can use Z.AI's coding surface.""" + agent = "openai-protocol-probe" + monkeypatch.setitem( + AGENTS, + agent, + AgentConfig(agent, "true", "true", api_protocol="openai-completions"), + ) env = resolve_agent_env(agent, "zai-coding/glm-5.3", {"ZAI_API_KEY": "native-key"}) route = resolve_litellm_route("zai-coding/glm-5.3", env) - assert env["BENCHFLOW_PROVIDER_BASE_URL"] == agent_base + assert env["BENCHFLOW_PROVIDER_BASE_URL"] == "https://api.z.ai/api/coding/paas/v4" assert route.litellm_params["api_base"] == ("https://api.z.ai/api/coding/paas/v4") assert route.litellm_params["api_key"] == "os.environ/ZAI_API_KEY" assert route.required_env == ("ZAI_API_KEY",) diff --git a/tests/test_litellm_runtime.py b/tests/test_litellm_runtime.py index 80d7d1b94..5cda4b36e 100644 --- a/tests/test_litellm_runtime.py +++ b/tests/test_litellm_runtime.py @@ -3,6 +3,7 @@ import json from importlib.metadata import version from types import SimpleNamespace +from unittest.mock import Mock import pytest from packaging.requirements import Requirement @@ -244,7 +245,12 @@ async def fake_start(**kwargs): @pytest.mark.asyncio async def test_pi_acp_proxy_preserves_provider_model_metadata(monkeypatch): - """Guards PR #803: Pi metadata follows the LiteLLM alias in proxy mode.""" + """Guards PRs #803/#1093: supplied metadata wins over catalog lookup.""" + import litellm + + monkeypatch.setattr( + litellm, "get_model_info", lambda *_: pytest.fail("unexpected catalog lookup") + ) async def fake_start(**kwargs): return FakeLiteLLMServer("http://172.17.0.1:45678", kwargs["route"]) @@ -278,10 +284,14 @@ async def fake_start(**kwargs): assert provider_runtime is not None assert updated["BENCHFLOW_PROVIDER_MODEL"] == "benchflow-vllm-Qwen-Qwen3-4B" models = json.loads(updated["BENCHFLOW_PROVIDER_MODELS"]) - alias = next(m for m in models if m["id"] == "benchflow-vllm-Qwen-Qwen3-4B") - assert alias["name"] == "benchflow-vllm-Qwen-Qwen3-4B" - assert alias["maxTokens"] == 1024 - assert alias["contextWindow"] == 16384 + assert models == [ + provider_models[0], + { + **provider_models[0], + "id": "benchflow-vllm-Qwen-Qwen3-4B", + "name": "benchflow-vllm-Qwen-Qwen3-4B", + }, + ] @pytest.mark.asyncio @@ -319,10 +329,10 @@ async def fake_start(**kwargs): assert created[0].stopped is True -@pytest.mark.parametrize("agent", ["claude-agent-acp", "openclaw"]) @pytest.mark.asyncio -async def test_zai_runtime_reconnect_preserves_upstream_route(monkeypatch, agent): +async def test_zai_runtime_reconnect_preserves_upstream_route(monkeypatch): """Guards PR #1074: reconnects retain Z.AI upstream routing and auth.""" + agent = "claude-agent-acp" starts = [] async def fake_start(**kwargs): @@ -819,3 +829,62 @@ async def fail_start(**_kwargs): assert updated == env assert provider_runtime is None + + +def test_proxy_alias_metadata_is_reused_after_catalog_resolution(monkeypatch): + """Guards PR #1093: resolve capabilities once, preserve caller alias metadata.""" + import litellm + + route = runtime_mod.resolve_litellm_route("openai/gpt-5.5", {}) + model_info = Mock( + return_value={ + "supports_reasoning": True, + "max_output_tokens": 128000, + "max_input_tokens": 1050000, + "supports_vision": True, + "supported_openai_params": ["reasoning_effort"], + } + ) + monkeypatch.setattr(litellm, "get_model_info", model_info) + params = dict( + agent="external-openclaw", + route=route, + base_url="http://proxy.test", + master_key="test-key", + ) + updated = runtime_mod._wire_litellm_agent_env(agent_env={}, **params) + models = json.loads(updated["BENCHFLOW_PROVIDER_MODELS"]) + assert models == [ + { + "id": route.model_alias, + "name": route.model_alias, + "reasoning": True, + "maxTokens": 128000, + "contextWindow": 1050000, + "input": ["text", "image"], + "compat": {"supportsReasoningEffort": True}, + } + ] + models[0].update( + reasoning=False, maxTokens=123, compat={"supportsReasoningEffort": False} + ) + updated["BENCHFLOW_PROVIDER_MODELS"] = json.dumps(models) + rewired = runtime_mod._wire_litellm_agent_env(agent_env=updated, **params) + assert rewired["BENCHFLOW_PROVIDER_MODELS"] == updated["BENCHFLOW_PROVIDER_MODELS"] + model_info.assert_called_once_with(route.upstream_model) + + +@pytest.mark.parametrize( + "outcome", + [ + Exception("This model isn't mapped yet"), + {"supports_reasoning": None, "max_output_tokens": -1}, + ], +) +def test_unknown_proxy_metadata_keeps_existing_behavior(monkeypatch, outcome): + """Guards PR #1093: absent catalog metadata does not block custom models.""" + import litellm + + monkeypatch.setattr(litellm, "get_model_info", Mock(side_effect=[outcome])) + route = runtime_mod.resolve_litellm_route("openai/custom-model", {}) + assert runtime_mod._provider_models_for_proxy_alias(raw=None, route=route) is None diff --git a/tests/test_providers.py b/tests/test_providers.py index 387da1264..5619a45a3 100644 --- a/tests/test_providers.py +++ b/tests/test_providers.py @@ -337,7 +337,7 @@ def test_is_vertex_model_zai_direct(self): assert is_vertex_model("zai/glm-5") is False -# Provider model metadata (for openclaw.json generation) +# Provider model metadata for agent configuration class TestProviderModels: @@ -392,7 +392,7 @@ def test_registered_prefix_with_huggingface_id(self): class TestShimProviderFallback: - """The openclaw shim must resolve providers from env vars when model is stripped. + """Agent shims must resolve providers from env vars when model is stripped. SDK strips provider prefix before ACP set_model (e.g. "anthropic-vertex/claude-sonnet-4-6" → "claude-sonnet-4-6"). The shim's _find_and_setup_provider() must fall through from @@ -429,126 +429,3 @@ def test_vllm_uses_openai_compatible_api_key(self): cfg = PROVIDERS["vllm"] assert cfg.auth_type == "api_key" assert cfg.auth_env == "OPENAI_API_KEY" - - -# Shim helper functions - - -class TestInferProviderPrefix: - """Tests for _infer_provider_prefix() in the openclaw ACP shim.""" - - @pytest.fixture(autouse=True) - def _import(self): - from benchflow.agents.openclaw_acp_shim import _infer_provider_prefix - - self.infer = _infer_provider_prefix - - @pytest.mark.parametrize( - "model,expected", - [ - ("gpt-4o", "openai"), - ("gpt-4o-mini", "openai"), - ("o1-preview", "openai"), - ("o3-mini", "openai"), - ("gemini-3-flash", "google"), - ("gemini-2.5-pro", "google"), - ("claude-sonnet-4-6", "anthropic"), - ("claude-haiku-4-5-20251001", "anthropic"), - ("some-unknown-model", "anthropic"), # default - ], - ) - def test_infer(self, model, expected): - assert self.infer(model) == expected - - -class TestSetupOpenaiAuth: - """Tests for setup_openai_auth() writing to openclaw's auth-profiles.json.""" - - @pytest.fixture() - def home_dir(self, tmp_path, monkeypatch): - monkeypatch.setenv("HOME", str(tmp_path)) - return tmp_path - - def _auth_path(self, home_dir): - return ( - home_dir / ".openclaw" / "agents" / "main" / "agent" / "auth-profiles.json" - ) - - def test_writes_key(self, home_dir, monkeypatch): - import json - - from benchflow.agents.openclaw_acp_shim import setup_openai_auth - - monkeypatch.setenv("OPENAI_API_KEY", "sk-test-123") - setup_openai_auth() - - auth = json.loads(self._auth_path(home_dir).read_text()) - assert auth["openai"]["apiKey"] == "sk-test-123" - - def test_no_key_is_noop(self, home_dir, monkeypatch): - from benchflow.agents.openclaw_acp_shim import setup_openai_auth - - monkeypatch.delenv("OPENAI_API_KEY", raising=False) - setup_openai_auth() - - assert not self._auth_path(home_dir).exists() - - def test_preserves_existing_providers(self, home_dir, monkeypatch): - import json - - from benchflow.agents.openclaw_acp_shim import setup_openai_auth - - path = self._auth_path(home_dir) - path.parent.mkdir(parents=True) - path.write_text(json.dumps({"anthropic": {"apiKey": "ant-key"}})) - - monkeypatch.setenv("OPENAI_API_KEY", "sk-test-456") - setup_openai_auth() - - auth = json.loads(path.read_text()) - assert auth["anthropic"]["apiKey"] == "ant-key" - assert auth["openai"]["apiKey"] == "sk-test-456" - - -# Shim model generation parameters - - -class TestShimModelParams: - """The shim should read BENCHFLOW_MODEL_* env vars and call - openclaw config set agents.defaults.params. for each.""" - - def test_param_map_covers_all_generation_params(self): - """session/set_model handler should map all three env vars. - - Parses the AST of openclaw_acp_shim.py to find the _PARAM_MAP dict - literal and assert its contents directly (not via source-text grep). - """ - import ast - from pathlib import Path - - shim_path = ( - Path(__file__).parent.parent / "src/benchflow/agents/openclaw_acp_shim.py" - ) - tree = ast.parse(shim_path.read_text()) - - param_map: dict[str, str] | None = None - for node in ast.walk(tree): - if ( - isinstance(node, ast.Assign) - and len(node.targets) == 1 - and isinstance(node.targets[0], ast.Name) - and node.targets[0].id == "_PARAM_MAP" - and isinstance(node.value, ast.Dict) - ): - param_map = { - ast.literal_eval(k): ast.literal_eval(v) - for k, v in zip(node.value.keys, node.value.values, strict=True) - } - break - - assert param_map is not None, "_PARAM_MAP not found in openclaw_acp_shim.py" - assert param_map == { - "BENCHFLOW_MODEL_TEMPERATURE": "agents.defaults.params.temperature", - "BENCHFLOW_MODEL_TOP_P": "agents.defaults.params.topP", - "BENCHFLOW_MODEL_MAX_TOKENS": "agents.defaults.params.maxTokens", - } diff --git a/tests/test_registry_invariants.py b/tests/test_registry_invariants.py index efeb025be..b6784ac90 100644 --- a/tests/test_registry_invariants.py +++ b/tests/test_registry_invariants.py @@ -130,13 +130,12 @@ def test_js_acp_agents_use_isolated_node_runtime(name): launch_cmd = AGENTS[name].launch_cmd assert "/opt/benchflow/node" in install_cmd - # Node >=22.19 is required by current openclaw (JS agents install @latest); - # assert the floor, not a brittle exact pin (BF-10). + # Assert shared runtime floor, not a brittle exact pin (BF-10). pin = re.search(r"BF_NODE_VERSION=(\d+)\.(\d+)\.\d+", install_cmd) assert pin, "BF_NODE_VERSION pin missing from JS agent bootstrap" major, minor = int(pin.group(1)), int(pin.group(2)) assert (major, minor) >= (22, 19), ( - f"pinned node {pin.group(0)} is below openclaw's >=22.19 floor" + f"pinned node {pin.group(0)} is below supported >=22.19 floor" ) assert "/opt/benchflow/js-agents" in install_cmd if name in DIRECT_JS_ACP_AGENTS: @@ -177,7 +176,6 @@ def test_js_acp_agents_use_isolated_node_runtime(name): "/usr/local/bin/npm", "/usr/local/bin/npx", "/usr/local/bin/pi-acp-launcher", - "/usr/local/bin/openclaw-acp-shim", "command -v node", ] for fragment in forbidden_fragments: @@ -186,11 +184,6 @@ def test_js_acp_agents_use_isolated_node_runtime(name): ) -def test_openclaw_is_pinned_to_node_22_20_compatibility(): - """Guards PR #704's Node 22.20.0 pin against floating OpenClaw releases.""" - assert "openclaw@2026.6.9" in AGENTS["openclaw"].install_cmd - - # Bash-isms not supported by dash (Ubuntu/Debian's /bin/sh). The sandbox # Docker/Daytona exec paths invoke ``sh -c install_cmd``; if /bin/sh is dash # (ubuntu:24.04 base), any of these aborts the install on line 1. See #341. @@ -330,14 +323,13 @@ def test_agent_derived_dicts_in_sync(): def test_agent_negative_config_invariants(): """Specific agents must NOT have certain features configured. - Tripwire for accidental config bleed (e.g. openclaw silently gaining - credential_files because someone copy-pasted from codex). Positive + Tripwire for accidental config bleed from copy-pasted fields. Positive per-agent assertions live in test_agent_registry.py / test_subscription_auth.py; this is the dedicated negative side. """ - no_credential_files = {"claude-agent-acp", "openclaw"} - no_subscription_auth = {"openclaw", "pi-acp"} - no_env_mapping = {"openclaw", "pi-acp"} + no_credential_files = {"claude-agent-acp"} + no_subscription_auth = {"pi-acp"} + no_env_mapping = {"pi-acp"} for name in no_credential_files: assert AGENTS[name].credential_files == [], ( diff --git a/tests/test_resolve_env_helpers.py b/tests/test_resolve_env_helpers.py index e28cbe1e7..60c8e4778 100644 --- a/tests/test_resolve_env_helpers.py +++ b/tests/test_resolve_env_helpers.py @@ -19,6 +19,7 @@ resolve_provider_env, validate_aws_bedrock_env, ) +from benchflow.agents.registry import AGENTS, AgentConfig # auto_inherit_env @@ -412,14 +413,15 @@ def test_bare_deepseek_resolves_provider_for_openhands(self): assert env["LLM_API_KEY"] == "dk-test" assert env["LLM_MODEL"] == "openai/deepseek-v4-pro" - def test_bare_deepseek_sets_provider_name_for_openclaw(self): - """Guards the harness-provider-resolution fix: openclaw must see - BENCHFLOW_PROVIDER_NAME=deepseek for a bare id, else its shim defaults the - provider to anthropic ('FailoverError: Unknown model: - anthropic/deepseek-v4-pro'). - """ + def test_bare_deepseek_sets_provider_name_for_generic_agent(self, monkeypatch): + """Guards PR #1090 generic bare-model provider resolution.""" + monkeypatch.setitem( + AGENTS, + "provider-probe", + AgentConfig("provider-probe", "true", "true"), + ) env = {"DEEPSEEK_API_KEY": "dk-test"} - resolve_provider_env(env, "deepseek-v4-pro", "openclaw") + resolve_provider_env(env, "deepseek-v4-pro", "provider-probe") assert env["BENCHFLOW_PROVIDER_NAME"] == "deepseek" assert env["BENCHFLOW_PROVIDER_BASE_URL"] == "https://api.deepseek.com/v1" assert env["BENCHFLOW_PROVIDER_API_KEY"] == "dk-test" @@ -479,9 +481,12 @@ def test_returns_false_for_unknown_agent(self): check_subscription_auth("nonexistent-agent", "ANTHROPIC_API_KEY") is False ) - def test_returns_false_when_no_subscription_auth(self): - """Agents without subscription_auth (e.g. openclaw) return False.""" - assert check_subscription_auth("openclaw", "ANTHROPIC_API_KEY") is False + def test_returns_false_when_no_subscription_auth(self, monkeypatch): + """Agents without subscription_auth return False.""" + monkeypatch.setitem( + AGENTS, "auth-probe", AgentConfig("auth-probe", "true", "true") + ) + assert check_subscription_auth("auth-probe", "ANTHROPIC_API_KEY") is False def test_codex_auth(self, monkeypatch, tmp_path): codex_dir = tmp_path / ".codex" @@ -630,7 +635,10 @@ def test_no_model_codex_api_key_alias_normalizes(self, monkeypatch, tmp_path): assert "_BENCHFLOW_SUBSCRIPTION_AUTH" not in result def test_no_model_empty_requires_env(self, monkeypatch, tmp_path): - """Agent with empty requires_env (e.g. openclaw) needs no auth.""" + """Agent with empty requires_env needs no auth.""" + monkeypatch.setitem( + AGENTS, "auth-probe", AgentConfig("auth-probe", "true", "true") + ) for k in ( "ANTHROPIC_API_KEY", "CODEX_ACCESS_TOKEN", @@ -639,7 +647,7 @@ def test_no_model_empty_requires_env(self, monkeypatch, tmp_path): ): monkeypatch.delenv(k, raising=False) self._patch_expanduser(monkeypatch, tmp_path) - result = self._resolve(agent="openclaw", agent_env={}) + result = self._resolve(agent="auth-probe", agent_env={}) assert "_BENCHFLOW_SUBSCRIPTION_AUTH" not in result diff --git a/tests/test_rollout_branch.py b/tests/test_rollout_branch.py index 79029cce3..2b1e25838 100644 --- a/tests/test_rollout_branch.py +++ b/tests/test_rollout_branch.py @@ -41,7 +41,9 @@ async def restore(self, snap: StateSnapshot) -> None: def _rollout(tmp_path: Path) -> Rollout: return Rollout( - RolloutConfig(task_path=tmp_path / "task", scenes=[Scene.single(agent="dummy")]) + RolloutConfig( + task_path=tmp_path / "task", scenes=[Scene.single(agent="dummy --agent")] + ) ) diff --git a/tests/test_rollout_planes_contract.py b/tests/test_rollout_planes_contract.py index 152c2400b..3143f56d5 100644 --- a/tests/test_rollout_planes_contract.py +++ b/tests/test_rollout_planes_contract.py @@ -85,16 +85,20 @@ def test_agent_launch_appends_web_tool_suffix_when_disallowed() -> None: ) -def test_agent_launch_unknown_agent_falls_back_to_name() -> None: +def test_agent_launch_unknown_bare_agent_fails_closed() -> None: + """Guards PR #1090 rollout-plane launch bypass.""" + import pytest + planes = DefaultRolloutPlanes() - # An unknown agent has no launch mapping and no config: returns the name, - # and the disallow flag is a no-op (no config -> no suffix). - assert planes.agent_launch("not-a-real-agent", disallow_web_tools=False) == ( - "not-a-real-agent" - ) - assert planes.agent_launch("not-a-real-agent", disallow_web_tools=True) == ( - "not-a-real-agent" - ) + with pytest.raises(KeyError, match="Unknown agent"): + planes.agent_launch("not-a-real-agent", disallow_web_tools=False) + + +def test_agent_launch_explicit_raw_command_passes_through() -> None: + """Guards PR #1090 explicit rollout-plane command compatibility.""" + planes = DefaultRolloutPlanes() + command = "agent --flag" + assert planes.agent_launch(command, disallow_web_tools=True) == command def test_agent_config_delegates_to_registry() -> None: diff --git a/tests/test_rollout_session_factory_dispatch.py b/tests/test_rollout_session_factory_dispatch.py index 8a060f15f..8b0931282 100644 --- a/tests/test_rollout_session_factory_dispatch.py +++ b/tests/test_rollout_session_factory_dispatch.py @@ -62,7 +62,9 @@ def steps(self) -> list[dict]: def _rollout(tmp_path: Path) -> Rollout: return Rollout( - RolloutConfig(task_path=tmp_path / "task", scenes=[Scene.single(agent="dummy")]) + RolloutConfig( + task_path=tmp_path / "task", scenes=[Scene.single(agent="dummy --agent")] + ) ) diff --git a/tests/test_runtime.py b/tests/test_runtime.py index 77be53f7c..344a2ea69 100644 --- a/tests/test_runtime.py +++ b/tests/test_runtime.py @@ -37,10 +37,18 @@ def test_agent_alias_resolves_config_and_launch() -> None: assert a.launch_cmd != "codex" -def test_agent_unknown() -> None: +def test_agent_unknown_bare_launch_fails_closed() -> None: + """Guards PR #1090 runtime launch bypass.""" a = Agent(name="nonexistent-agent", model="some-model") assert a.config is None - assert a.launch_cmd == "nonexistent-agent" + with pytest.raises(KeyError, match="Unknown agent"): + _ = a.launch_cmd + + +def test_agent_explicit_raw_command_launches() -> None: + """Guards PR #1090 explicit runtime command compatibility.""" + command = "python agent.py" + assert Agent(name=command, model="some-model").launch_cmd == command def test_agent_env_default_empty() -> None: diff --git a/tests/test_runtime_live_sandbox.py b/tests/test_runtime_live_sandbox.py index 9f4c4a14d..ee345c022 100644 --- a/tests/test_runtime_live_sandbox.py +++ b/tests/test_runtime_live_sandbox.py @@ -18,6 +18,7 @@ from pathlib import Path from typing import Any +from unittest.mock import Mock import pytest @@ -128,6 +129,22 @@ def fake_create_environment(*args: Any, **kwargs: Any) -> Any: assert rollout._env_externally_owned is False +@pytest.mark.asyncio +async def test_setup_oracle_skips_agent_launch_resolution( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Guards PR #1093: oracle setup has no agent process to launch.""" + rollout = Rollout(RolloutConfig(task_path=TASK_PATH, agent="oracle")) + rollout.use_prebuilt_env(_FakeInner()) + agent_launch = Mock(side_effect=AssertionError("oracle has no agent launch")) + monkeypatch.setattr(rollout._planes, "agent_launch", agent_launch) + + await rollout.setup() + + agent_launch.assert_not_called() + assert rollout._agent_launch == "" + + @pytest.mark.asyncio async def test_cleanup_does_not_stop_externally_owned_env( monkeypatch: pytest.MonkeyPatch, diff --git a/tests/test_sandbox.py b/tests/test_sandbox.py index e69370152..32a2ba569 100644 --- a/tests/test_sandbox.py +++ b/tests/test_sandbox.py @@ -28,12 +28,6 @@ def test_credential_file_dirs_included(self): # codex-acp has credential_file at {home}/.codex/auth.json assert ".codex" in dirs - def test_home_dirs_included(self): - """Explicit home_dirs from AgentConfig are included.""" - dirs = get_sandbox_home_dirs() - # openclaw has home_dirs=[".openclaw"] - assert ".openclaw" in dirs - def test_does_not_include_legacy_local_tool_dir(self): """.local is not included unless an agent registry path derives it.""" dirs = get_sandbox_home_dirs() @@ -47,7 +41,7 @@ def test_only_includes_top_level_home_dirs(self): def test_dirs_represent_registry_backed_home_config_or_auth(self): """Returned dirs are registry-derived user home config/auth roots.""" dirs = get_sandbox_home_dirs() - assert {".claude", ".codex", ".gemini", ".openclaw"}.issubset(dirs) + assert {".claude", ".codex", ".gemini"}.issubset(dirs) assert ".agents" not in dirs assert ".pi" not in dirs @@ -91,7 +85,6 @@ def test_subscription_auth_file_dirs_included(self): def test_workspace_paths_excluded(self): """$WORKSPACE paths are not included (only $HOME paths).""" dirs = get_sandbox_home_dirs() - # openclaw has $WORKSPACE/skills — should NOT produce a dir entry assert "skills" not in dirs def test_returns_set_of_strings(self): diff --git a/tests/test_sdk_internals.py b/tests/test_sdk_internals.py index df571c90c..5f192e0db 100644 --- a/tests/test_sdk_internals.py +++ b/tests/test_sdk_internals.py @@ -196,10 +196,17 @@ def test_provider_bridge_key_alone_does_not_bypass_required_model_key( self, monkeypatch ): """Only mapped agent-native keys can bypass provider-specific key checks.""" + from benchflow.agents.registry import AGENTS, AgentConfig + + monkeypatch.setitem( + AGENTS, + "auth-probe", + AgentConfig("auth-probe", "true", "true"), + ) monkeypatch.delenv("OPENAI_API_KEY", raising=False) with pytest.raises(ValueError, match="OPENAI_API_KEY required"): self._resolve( - agent="openclaw", + agent="auth-probe", model="openai/gpt-4.1-mini", agent_env={"BENCHFLOW_PROVIDER_API_KEY": "x"}, ) diff --git a/tests/test_task_runtime_primitive.py b/tests/test_task_runtime_primitive.py index 6b2658fc0..a11c53e22 100644 --- a/tests/test_task_runtime_primitive.py +++ b/tests/test_task_runtime_primitive.py @@ -218,6 +218,23 @@ async def cleanup_verifier_python_hooks(self, *args: Any, **kwargs: Any) -> None return None +@pytest.mark.asyncio +async def test_task_runtime_default_setup_does_not_resolve_an_agent_launch( + tmp_path: Path, +) -> None: + """Guards PR #1093 from making the non-agent task runtime fail closed.""" + rollout = Rollout( + TaskRuntimeConfig( + task_path=TASK_PATH, + jobs_dir=tmp_path / "jobs", + ).to_rollout_config() + ) + + await rollout.setup() + + assert rollout._agent_launch == "" + + @pytest.mark.asyncio async def test_task_runtime_bash_verify_writes_rollout_artifacts( tmp_path: Path, diff --git a/tests/test_trial_agent_timeout_verify.py b/tests/test_trial_agent_timeout_verify.py index d319ad6d6..0ed7777c8 100644 --- a/tests/test_trial_agent_timeout_verify.py +++ b/tests/test_trial_agent_timeout_verify.py @@ -28,7 +28,7 @@ async def test_agent_timeout_verifier_result_handling( """Guards the reward-output regression on v0.5-integration@ffef85d.""" cfg = RolloutConfig( task_path=tmp_path / "task", - scenes=[Scene.single(agent="dummy")], + scenes=[Scene.single(agent="dummy --agent")], ) trial = Rollout(cfg) calls: list[str] = [] diff --git a/tests/test_verify.py b/tests/test_verify.py index d3821839b..c420a93fd 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -1147,12 +1147,12 @@ async def test_scraped_trajectory_preserves_n_tool_calls( mock_acp, mock_session, MagicMock(), - "test-agent", + "test-agent --run", ) planes.execute_prompts.return_value = ([], 5) result = await sdk.run( task_dir, - agent="test-agent", + agent="test-agent --run", agent_env={"TEST": "1"}, sandbox_user=None, jobs_dir=task_dir.parent / "jobs", @@ -1206,12 +1206,12 @@ async def test_partial_acp_uses_session_tool_calls(self, sdk_run_mocks): mock_acp, mock_session, MagicMock(), - "test-agent", + "test-agent --run", ) planes.execute_prompts.side_effect = ConnectionError("lost") result = await sdk.run( task_dir, - agent="test-agent", + agent="test-agent --run", agent_env={"TEST": "1"}, sandbox_user=None, jobs_dir=task_dir.parent / "jobs", diff --git a/tests/trajectories/test_step_granularity.py b/tests/trajectories/test_step_granularity.py index 84cb5bdf2..17be25c37 100644 --- a/tests/trajectories/test_step_granularity.py +++ b/tests/trajectories/test_step_granularity.py @@ -19,7 +19,9 @@ def _rollout(tmp_path: Path) -> Rollout: return Rollout( - RolloutConfig(task_path=tmp_path / "task", scenes=[Scene.single(agent="dummy")]) + RolloutConfig( + task_path=tmp_path / "task", scenes=[Scene.single(agent="dummy --agent")] + ) )