diff --git a/clawloop/cli.py b/clawloop/cli.py index 8d2e24ed..782157e3 100644 --- a/clawloop/cli.py +++ b/clawloop/cli.py @@ -161,30 +161,105 @@ def cmd_run(args: argparse.Namespace) -> None: train(config) -def _install_dry_run_clients(config: "Any") -> None: - """Patch ``clawloop.train._make_llm_client`` to return mock clients. - - Identifies the role (reflector / task / other) by matching the cfg - object identity against ``config.llm_clients``. Falls back to a generic - ``MockLLMClient`` for any unknown role so unfamiliar envs still run. +def _install_dry_run_clients(config: Any) -> None: + """Wire `--dry-run`: guarantee no real LLM calls regardless of env_type. + + Two parts: + 1. Stamp ``dry_run_role`` on each ``LLMClientConfig`` and patch + ``clawloop.train._make_llm_client`` to switch on that field. The + role travels with the data, so it survives Pydantic ``model_copy()`` + — a failure mode the earlier ``id(cfg)`` approach was vulnerable to. + A dedicated field (vs. overloading ``model``) keeps the public + ``model`` value pristine for any code that reads it downstream. + 2. For envs whose adapter bypasses ``_make_llm_client`` (per + ``train.ENVS_USING_MAKE_LLM_CLIENT``), swap the registered builder + with a stub that returns a no-I/O ``_StubAdapter``. """ import clawloop.train as _train from clawloop.demo_math import MockTaskClient, _build_mock_reflector_responses from clawloop.llm import MockLLMClient - role_by_id = {id(v): k for k, v in config.llm_clients.items()} - original = _train._make_llm_client + # Part 1: tag each LLMClientConfig with its role, then route + # _make_llm_client through a mock factory that reads the tag. + for role, cfg in config.llm_clients.items(): + cfg.dry_run_role = role + + original_make = _train._make_llm_client def _mock_make(cfg): - role = role_by_id.get(id(cfg)) - if role == "reflector": - return MockLLMClient(responses=_build_mock_reflector_responses()) - if role == "task": - return MockTaskClient() - return MockLLMClient(responses=["[]"]) + role = getattr(cfg, "dry_run_role", None) + if role is not None: + if role == "reflector": + return MockLLMClient(responses=_build_mock_reflector_responses()) + if role == "task": + return MockTaskClient() + return MockLLMClient(responses=["[]"]) + return original_make(cfg) _train._make_llm_client = _mock_make - log.info("dry-run: LLM clients patched to mocks (original=%r)", original.__name__) + + # Part 2: for env_types that bypass _make_llm_client, replace the + # registered builder with one that returns a stub adapter. Without this, + # --dry-run on (e.g.) taubench / entropic / openclaw would still hit + # real endpoints. + env_type = config.env_type + uses_make_llm_client = env_type in _train.ENVS_USING_MAKE_LLM_CLIENT + if not uses_make_llm_client and env_type in _train.ENV_BUILDERS: + # Floor at 1 to keep the task list non-empty even if a config sets + # episodes_per_iter to 0; the learning loop samples from it. + n_tasks = max(1, config.episodes_per_iter) + stub_tasks = [f"dry_run_{env_type}_{i}" for i in range(n_tasks)] + + def _stub_builder(_cfg: Any, _clients: Any) -> tuple[Any, list[str]]: + return _StubAdapter(env_type), list(stub_tasks) + + _train.ENV_BUILDERS[env_type] = _stub_builder + + log.info( + "dry-run: LLM clients mocked; env=%r %s", + env_type, + "uses _make_llm_client" if uses_make_llm_client else "stubbed", + ) + + +class _StubAdapter: + """Adapter that yields canned episodes — no network, no LLM calls. + + Used by --dry-run for env_types whose real adapter would otherwise + drive external services (tau2, CRMArena, OpenClaw, OpenSpiel). + """ + + def __init__(self, env_type: str) -> None: + self._env_type = env_type + + def run_episode(self, task: Any, agent_state: Any) -> Any: + from uuid import uuid4 + + from clawloop.core.episode import Episode, EpisodeSummary, StepMeta + + state_id = "" + try: + state_id = agent_state.state_id().combined_hash + except Exception: + pass + + return Episode( + id=uuid4().hex, + state_id=state_id, + task_id=f"{self._env_type}:{task}", + bench=self._env_type, + messages=[], + step_boundaries=[], + steps=[StepMeta(t=0, reward=1.0, done=True, timing_ms=0.0)], + summary=EpisodeSummary(total_reward=1.0), + metadata={"dry_run": True}, + ) + + def run_batch(self, agent_state: Any, task_ids: list[Any]) -> list[Any]: + return [self.run_episode(t, agent_state) for t in task_ids] + + def get_traces(self, episode: Any) -> dict[str, Any]: + return {"bench": self._env_type, "episode_id": episode.id, "dry_run": True} def main() -> None: diff --git a/clawloop/train.py b/clawloop/train.py index ec7324a9..d85a329d 100644 --- a/clawloop/train.py +++ b/clawloop/train.py @@ -36,6 +36,11 @@ class LLMClientConfig(BaseModel): temperature: float = 0.7 max_tokens: int = 2000 + # Internal marker used by `clawloop run --dry-run` to route this client to + # a mock without mutating `model`. Survives Pydantic `model_copy()`. + # Always None for normal training runs. + dry_run_role: str | None = None + model_config = {"arbitrary_types_allowed": True} @@ -169,7 +174,9 @@ def _build_openclaw(config: TrainConfig, llm_clients: dict[str, LLMClientConfig] return adapter, tasks -def _build_taubench(config: TrainConfig, llm_clients: dict[str, LLMClientConfig]) -> tuple: +def _build_taubench( + config: TrainConfig, llm_clients: dict[str, LLMClientConfig] +) -> tuple[Any, list[str]]: from clawloop.environments.taubench import TauBenchAdapter taubench_cfg = dict(config.env_config or {}) @@ -249,6 +256,17 @@ def _build_openspiel(config: "TrainConfig", llm_clients: dict[str, "LLMClientCon "taubench": _build_taubench, } +# Env types whose builder routes its task LLM through `_make_llm_client`, +# so patching that helper alone is enough to stop real network calls under +# `clawloop run --dry-run`. Every other env_type drives LLMs internally +# (e.g. tau2 inside taubench, EntropicAdapter.setup), and the CLI will +# install a `_StubAdapter` for it instead. +# +# Maintenance: when registering a new builder above, decide whether it +# calls `_make_llm_client`. If yes, add the env_type here. If no, leave +# it out — `--dry-run` will fall back to the stub adapter. +ENVS_USING_MAKE_LLM_CLIENT: frozenset[str] = frozenset({"math"}) + # --------------------------------------------------------------------------- # Validation @@ -343,6 +361,35 @@ def validate_config(config: TrainConfig) -> list[str]: if config.env_type == "entropic": if not config.env_config: raise ValueError("entropic env requires 'env_config'") + if config.env_type == "taubench": + if not config.env_config: + raise ValueError("taubench env requires 'env_config'") + tb = config.env_config + + # Validate only keys the user supplied; TauBenchAdapter.setup owns + # the defaults for any key they omit, so duplicating them here would + # split that knowledge across two files. + def _positive_int(key: str) -> None: + if key not in tb: + return + v = tb[key] + # Reject bool and float explicitly: `int(True) == 1` and + # `int(3.5) == 3` would otherwise pass silently, masking bad + # configs (e.g. `num_tasks: true` or `max_steps: 3.5`). + if isinstance(v, (bool, float)): + raise ValueError(f"taubench env_config.{key} must be a positive int (got {v!r})") + try: + iv = int(v) + except (TypeError, ValueError) as exc: + raise ValueError( + f"taubench env_config.{key} must be a positive int (got {v!r})" + ) from exc + if iv <= 0: + raise ValueError(f"taubench env_config.{key} must be a positive int (got {iv})") + + _positive_int("num_tasks") + _positive_int("max_steps") + _positive_int("max_concurrency") if config.env_type == "openspiel": # OpenSpielTaskEnvironment.run_episode reads sampling_client / # renderer / tokenizer off AgentState — those are only populated diff --git a/tests/test_cli.py b/tests/test_cli.py index cdc7be3c..ac4ef9cb 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -106,3 +106,97 @@ def test_run_missing_config_errors(): # Should be a FileNotFoundError, not the old disabled-redirect text. combined = result.stdout + result.stderr assert "train_runner.py" not in combined + + +# --------------------------------------------------------------------------- +# --dry-run on envs that bypass _make_llm_client (PR #60 review comment 1) +# --------------------------------------------------------------------------- +# These envs (taubench, entropic, openclaw) construct or drive LLM calls +# inside their adapter rather than via train._make_llm_client. Stubbing the +# env builder under --dry-run is what makes the flag actually safe — no +# real API calls regardless of env_type or presence of API keys. + + +def _write_tiny(tmp_path: Path, src: Path) -> Path: + raw = json.loads(src.read_text()) + raw["n_iterations"] = 1 + raw["episodes_per_iter"] = 1 + out = tmp_path / src.name + out.write_text(json.dumps(raw)) + return out + + +def test_run_taubench_dry_run_no_api_calls(tmp_path: Path, monkeypatch): + """Even without tau2 / API keys, --dry-run on taubench must succeed + via the stub adapter and never touch the real ENV_BUILDER.""" + monkeypatch.delenv("OPENAI_API_KEY", raising=False) + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + monkeypatch.delenv("GEMINI_API_KEY", raising=False) + cfg_path = _write_tiny(tmp_path, CONFIGS_DIR / "taubench_harness.json") + result = _run_cli("run", str(cfg_path), "--dry-run") + assert result.returncode == 0, f"taubench dry-run failed: {result.stderr}" + + +def test_run_entropic_dry_run_no_api_calls(tmp_path: Path, monkeypatch): + monkeypatch.delenv("OPENAI_API_KEY", raising=False) + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + cfg_path = _write_tiny(tmp_path, CONFIGS_DIR / "entropic_harness.json") + result = _run_cli("run", str(cfg_path), "--dry-run") + assert result.returncode == 0, f"entropic dry-run failed: {result.stderr}" + + +def test_run_openclaw_dry_run_no_api_calls(tmp_path: Path, monkeypatch): + monkeypatch.delenv("OPENAI_API_KEY", raising=False) + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + cfg_path = _write_tiny(tmp_path, CONFIGS_DIR / "openclaw_proxy.json") + result = _run_cli("run", str(cfg_path), "--dry-run") + assert result.returncode == 0, f"openclaw dry-run failed: {result.stderr}" + + +# --------------------------------------------------------------------------- +# Role marker survives Pydantic copy (PR #60 review comment 2) +# --------------------------------------------------------------------------- + + +def test_dry_run_role_field_survives_pydantic_copy(monkeypatch): + """The role lives in a dedicated `dry_run_role` field so it survives + `.model_copy()`, and `model` stays unmodified for downstream code + (e.g. `_build_entropic` reads `tc.model` directly). Regression for + PR #60 review comment 2 and PR #62 follow-up.""" + import clawloop.cli as _cli + import clawloop.train as _train + from clawloop.demo_math import MockTaskClient + from clawloop.llm import MockLLMClient + from clawloop.train import LLMClientConfig, TrainConfig + + original_make = _train._make_llm_client + + cfg = TrainConfig( + mode="harness_learning", + env_type="math", + llm_clients={ + "reflector": LLMClientConfig(model="anthropic/claude-sonnet-4"), + "task": LLMClientConfig(model="anthropic/claude-haiku-4"), + }, + ) + + _cli._install_dry_run_clients(cfg) + try: + # `model` is preserved verbatim — no marker pollution. + assert cfg.llm_clients["reflector"].model == "anthropic/claude-sonnet-4" + assert cfg.llm_clients["task"].model == "anthropic/claude-haiku-4" + assert cfg.llm_clients["reflector"].dry_run_role == "reflector" + assert cfg.llm_clients["task"].dry_run_role == "task" + + # Simulate Pydantic revalidation / copy: address changes, but the + # role field travels in the data and is preserved. + copied_reflector = cfg.llm_clients["reflector"].model_copy() + copied_task = cfg.llm_clients["task"].model_copy() + assert id(copied_reflector) != id(cfg.llm_clients["reflector"]) + assert copied_reflector.dry_run_role == "reflector" + assert copied_task.dry_run_role == "task" + + assert isinstance(_train._make_llm_client(copied_reflector), MockLLMClient) + assert isinstance(_train._make_llm_client(copied_task), MockTaskClient) + finally: + _train._make_llm_client = original_make diff --git a/tests/test_train_config.py b/tests/test_train_config.py index aad75a58..446f1ba1 100644 --- a/tests/test_train_config.py +++ b/tests/test_train_config.py @@ -121,6 +121,85 @@ def test_harbor_empty_dirs_fails(self): validate_config(cfg) +class TestTauBenchValidation: + """Acceptance for PR #60 review comment 4: validate_config must catch + bad taubench env_config up-front instead of failing deep in tau2 / the + adapter. Each knob with a meaningful positivity constraint is enforced.""" + + @staticmethod + def _base(env_config: dict | None = None) -> TrainConfig: + return TrainConfig( + mode="harness_learning", + env_type="taubench", + llm_clients=_llm("reflector"), + env_config=env_config, + ) + + def test_requires_env_config(self): + cfg = self._base(env_config=None) + with pytest.raises(ValueError, match="env_config"): + validate_config(cfg) + + def test_num_tasks_zero_rejected(self): + cfg = self._base({"num_tasks": 0}) + with pytest.raises(ValueError, match="num_tasks"): + validate_config(cfg) + + def test_num_tasks_negative_rejected(self): + cfg = self._base({"num_tasks": -1}) + with pytest.raises(ValueError, match="num_tasks"): + validate_config(cfg) + + def test_num_tasks_omitted_ok(self): + cfg = self._base({"domain": "retail"}) + assert validate_config(cfg) == ["harness", "router"] + + def test_max_steps_zero_rejected(self): + cfg = self._base({"max_steps": 0}) + with pytest.raises(ValueError, match="max_steps"): + validate_config(cfg) + + def test_max_concurrency_zero_rejected(self): + cfg = self._base({"max_concurrency": 0}) + with pytest.raises(ValueError, match="max_concurrency"): + validate_config(cfg) + + def test_full_valid_config_ok(self): + cfg = self._base( + { + "domain": "retail", + "num_tasks": 3, + "max_steps": 30, + "max_concurrency": 8, + } + ) + assert validate_config(cfg) == ["harness", "router"] + + @pytest.mark.parametrize("value", [True, False]) + def test_num_tasks_bool_rejected(self, value): + """`int(True) == 1` would otherwise sneak past — explicit reject.""" + cfg = self._base({"num_tasks": value}) + with pytest.raises(ValueError, match="num_tasks"): + validate_config(cfg) + + @pytest.mark.parametrize("value", [3.5, 1.0, 0.0]) + def test_num_tasks_float_rejected(self, value): + """`int(3.5) == 3` silently truncates — explicit reject.""" + cfg = self._base({"num_tasks": value}) + with pytest.raises(ValueError, match="num_tasks"): + validate_config(cfg) + + def test_max_steps_float_rejected(self): + cfg = self._base({"max_steps": 30.0}) + with pytest.raises(ValueError, match="max_steps"): + validate_config(cfg) + + def test_max_concurrency_bool_rejected(self): + cfg = self._base({"max_concurrency": True}) + with pytest.raises(ValueError, match="max_concurrency"): + validate_config(cfg) + + # --------------------------------------------------------------------------- # LLMClientConfig # ---------------------------------------------------------------------------