fix: address PR #60 review comments on clawloop run --dry-run + taubench

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:

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

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

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
 # ---------------------------------------------------------------------------