From ff9a870505a5bbe044caf0ae4d64c7b554c59c21 Mon Sep 17 00:00:00 2001 From: dantp-ai <1534513+dantp-ai@users.noreply.github.com> Date: Wed, 10 Jun 2026 23:03:04 +0200 Subject: [PATCH 1/4] fix: address PR #60 review comments on `clawloop run --dry-run` and taubench MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #60 review (gemini-code-assist) flagged four issues; this commit addresses all four with tests. 1. `_install_dry_run_clients` only patched `_make_llm_client`, so envs that bypass it (entropic, openclaw, taubench) could still make real API calls under `--dry-run`. The function now also swaps the env's builder in `ENV_BUILDERS` for a stub adapter whose `run_episode` / `run_batch` return canned episodes — no I/O, no LLM calls. Math keeps the real adapter path since `MathAdapter` routes through `_make_llm_client`. 2. Role identification used `id(cfg)`, which breaks if Pydantic copies or revalidates the `LLMClientConfig`. Replaced with a marker baked into `cfg.model`; the marker travels in field data so it survives `model_copy()`. New regression test covers a `model_copy()` round-trip. 3. `_build_taubench` return type widened from bare `tuple` to `tuple[Any, list[str]]` to match the rest of the builder family and enable static analysis. 4. `validate_config` now enforces taubench env_config: `env_config` required; `num_tasks` / `max_steps` / `max_concurrency` (if set) must be positive ints — `0` is no longer silently accepted via falsiness. Six new validation tests. Verification: 1036 + 49 = full suite passes (24s); manual smoke runs on math, taubench (no API keys set), and a bad-config rejection all behave as expected. Co-Authored-By: Claude Opus 4.7 --- clawloop/cli.py | 121 ++++++++++++++++++++++++++++++++----- clawloop/train.py | 27 ++++++++- tests/test_cli.py | 85 ++++++++++++++++++++++++++ tests/test_train_config.py | 55 +++++++++++++++++ 4 files changed, 272 insertions(+), 16 deletions(-) diff --git a/clawloop/cli.py b/clawloop/cli.py index 8d2e24ed..f28c8070 100644 --- a/clawloop/cli.py +++ b/clawloop/cli.py @@ -161,30 +161,121 @@ 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. +# Marker baked into LLMClientConfig.model so `_make_llm_client` can recover +# the role after Pydantic copies / revalidation. Object identity (the prior +# id(cfg) approach) is fragile: any model_copy / re-instantiation breaks it. +_DRY_RUN_MARKER = "__clawloop_dry_run_role__:" + +# Envs whose adapter routes its task LLM through `_make_llm_client`. For +# every other env_type the adapter constructs / drives LLMs internally +# (e.g. tau2 inside taubench, EntropicAdapter.setup), so patching +# `_make_llm_client` alone wouldn't stop real network calls — we also have +# to install a stub builder. +_ENVS_USING_MAKE_LLM_CLIENT = frozenset({"math"}) + + +def _install_dry_run_clients(config: Any) -> None: + """Wire `--dry-run`: guarantee no real LLM calls regardless of env_type. + + Two parts: + 1. Embed the role into each ``LLMClientConfig.model`` field and patch + ``clawloop.train._make_llm_client`` to switch on that marker. The + marker travels with the data, so it survives Pydantic copies — a + failure mode the earlier ``id(cfg)`` approach was vulnerable to. + 2. For envs whose adapter bypasses ``_make_llm_client``, swap the + registered builder with a stub that returns a no-I/O adapter + whose ``run_episode`` / ``run_batch`` produce canned episodes. """ 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: mark each LLMClientConfig with its role, then route + # _make_llm_client through a mock factory that reads the marker. + for role, cfg in config.llm_clients.items(): + if not cfg.model.startswith(_DRY_RUN_MARKER): + cfg.model = f"{_DRY_RUN_MARKER}{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=["[]"]) + if cfg.model.startswith(_DRY_RUN_MARKER): + role = cfg.model[len(_DRY_RUN_MARKER) :] + 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 a stub. Without this, --dry-run on (e.g.) + # taubench / entropic / openclaw would still hit real endpoints. + env_type = config.env_type + if env_type not in _ENVS_USING_MAKE_LLM_CLIENT and env_type in _train.ENV_BUILDERS: + _train.ENV_BUILDERS[env_type] = _make_stub_env_builder(env_type) + + log.info( + "dry-run: LLM clients mocked; env=%r %s", + env_type, + "stubbed" if env_type not in _ENVS_USING_MAKE_LLM_CLIENT else "uses _make_llm_client", + ) + + +def _make_stub_env_builder(env_type: str): + """Return a builder that constructs a no-I/O stub adapter for `env_type`.""" + + def _build_stub(config: Any, llm_clients: Any) -> tuple[Any, list[str]]: + tasks = [f"dry_run_{env_type}_{i}" for i in range(3)] + return _StubAdapter(env_type), tasks + + return _build_stub + + +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: + return _stub_episode(self._env_type, task, agent_state) + + def run_batch(self, agent_state: Any, task_ids: list[Any]) -> list[Any]: + return [_stub_episode(self._env_type, 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 _stub_episode(env_type: str, 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"{env_type}:{task}", + bench=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 main() -> None: diff --git a/clawloop/train.py b/clawloop/train.py index ec7324a9..a3d32937 100644 --- a/clawloop/train.py +++ b/clawloop/train.py @@ -169,7 +169,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 {}) @@ -343,6 +345,29 @@ 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 + + def _positive_int(key: str, default: int | None = None) -> None: + v = tb.get(key, default) + if v is None: + return + 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", default=30) + _positive_int("max_concurrency", default=8) 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..be14deea 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -106,3 +106,88 @@ 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_marker_survives_pydantic_copy(monkeypatch): + """Replacing id(cfg) with a model-field marker means revalidation / + `.model_copy()` no longer breaks role lookup. Regression for PR #60 + review comment 2.""" + 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: + # Simulate Pydantic revalidation / copy: address changes, + # but the marker travels with the data. + 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 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..e521a644 100644 --- a/tests/test_train_config.py +++ b/tests/test_train_config.py @@ -121,6 +121,61 @@ 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"] + + # --------------------------------------------------------------------------- # LLMClientConfig # --------------------------------------------------------------------------- From b81403fc21b221104e2439d72f0739874dd28ecf Mon Sep 17 00:00:00 2001 From: dantp-ai <1534513+dantp-ai@users.noreply.github.com> Date: Wed, 10 Jun 2026 23:08:28 +0200 Subject: [PATCH 2/4] style: ruff-format clawloop/train.py Collapses a short `raise ValueError(...)` to a single line. Fix for the lint CI on PR #62; no behavior change. Co-Authored-By: Claude Opus 4.7 --- clawloop/train.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/clawloop/train.py b/clawloop/train.py index a3d32937..6326e1f2 100644 --- a/clawloop/train.py +++ b/clawloop/train.py @@ -361,9 +361,7 @@ def _positive_int(key: str, default: int | None = None) -> None: 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})" - ) + raise ValueError(f"taubench env_config.{key} must be a positive int (got {iv})") _positive_int("num_tasks") _positive_int("max_steps", default=30) From fba649fc1e4b8fb2562999a0c03ac1cfbd7effdc Mon Sep 17 00:00:00 2001 From: dantp-ai <1534513+dantp-ai@users.noreply.github.com> Date: Wed, 10 Jun 2026 23:18:38 +0200 Subject: [PATCH 3/4] fix: address PR #62 review comments (dry_run_role field, strict int check) Follow-up review on PR #62 raised two concerns; both are now fixed. 1. Storing the dry-run role by overloading `LLMClientConfig.model` could pollute downstream consumers that read the field as a real model name (e.g. `_build_entropic` propagates `tc.model` into `entropic_cfg`). Replaced the marker-in-model trick with a dedicated `dry_run_role: str | None = None` field on `LLMClientConfig`. The field travels with the data, so it survives `model_copy()` like the marker did, but leaves `model` untouched. Test now asserts both the field round-trip AND that `model` is unmodified after `_install_dry_run_clients`. 2. `_positive_int` in `validate_config` accepted `bool` and `float` silently because `int(True) == 1` and `int(3.5) == 3`. Added an explicit `isinstance(v, (bool, float))` reject before the `int(v)` coercion. Six new tests cover bool / float rejection on `num_tasks`, `max_steps`, and `max_concurrency`. Verification: pre-commit clean; 56 passes in test_cli.py + test_train_config.py. Co-Authored-By: Claude Opus 4.7 --- clawloop/cli.py | 26 +++++++++++--------------- clawloop/train.py | 10 ++++++++++ tests/test_cli.py | 21 +++++++++++++++------ tests/test_train_config.py | 24 ++++++++++++++++++++++++ 4 files changed, 60 insertions(+), 21 deletions(-) diff --git a/clawloop/cli.py b/clawloop/cli.py index f28c8070..c62ff2af 100644 --- a/clawloop/cli.py +++ b/clawloop/cli.py @@ -161,11 +161,6 @@ def cmd_run(args: argparse.Namespace) -> None: train(config) -# Marker baked into LLMClientConfig.model so `_make_llm_client` can recover -# the role after Pydantic copies / revalidation. Object identity (the prior -# id(cfg) approach) is fragile: any model_copy / re-instantiation breaks it. -_DRY_RUN_MARKER = "__clawloop_dry_run_role__:" - # Envs whose adapter routes its task LLM through `_make_llm_client`. For # every other env_type the adapter constructs / drives LLMs internally # (e.g. tau2 inside taubench, EntropicAdapter.setup), so patching @@ -178,10 +173,12 @@ def _install_dry_run_clients(config: Any) -> None: """Wire `--dry-run`: guarantee no real LLM calls regardless of env_type. Two parts: - 1. Embed the role into each ``LLMClientConfig.model`` field and patch - ``clawloop.train._make_llm_client`` to switch on that marker. The - marker travels with the data, so it survives Pydantic copies — a - failure mode the earlier ``id(cfg)`` approach was vulnerable to. + 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``, swap the registered builder with a stub that returns a no-I/O adapter whose ``run_episode`` / ``run_batch`` produce canned episodes. @@ -190,17 +187,16 @@ def _install_dry_run_clients(config: Any) -> None: from clawloop.demo_math import MockTaskClient, _build_mock_reflector_responses from clawloop.llm import MockLLMClient - # Part 1: mark each LLMClientConfig with its role, then route - # _make_llm_client through a mock factory that reads the marker. + # 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(): - if not cfg.model.startswith(_DRY_RUN_MARKER): - cfg.model = f"{_DRY_RUN_MARKER}{role}" + cfg.dry_run_role = role original_make = _train._make_llm_client def _mock_make(cfg): - if cfg.model.startswith(_DRY_RUN_MARKER): - role = cfg.model[len(_DRY_RUN_MARKER) :] + 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": diff --git a/clawloop/train.py b/clawloop/train.py index 6326e1f2..2c7c560a 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} @@ -354,6 +359,11 @@ def _positive_int(key: str, default: int | None = None) -> None: v = tb.get(key, default) if v is None: return + # 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: diff --git a/tests/test_cli.py b/tests/test_cli.py index be14deea..ac4ef9cb 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -158,10 +158,11 @@ def test_run_openclaw_dry_run_no_api_calls(tmp_path: Path, monkeypatch): # --------------------------------------------------------------------------- -def test_dry_run_role_marker_survives_pydantic_copy(monkeypatch): - """Replacing id(cfg) with a model-field marker means revalidation / - `.model_copy()` no longer breaks role lookup. Regression for 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 @@ -181,11 +182,19 @@ def test_dry_run_role_marker_survives_pydantic_copy(monkeypatch): _cli._install_dry_run_clients(cfg) try: - # Simulate Pydantic revalidation / copy: address changes, - # but the marker travels with the data. + # `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) diff --git a/tests/test_train_config.py b/tests/test_train_config.py index e521a644..446f1ba1 100644 --- a/tests/test_train_config.py +++ b/tests/test_train_config.py @@ -175,6 +175,30 @@ def test_full_valid_config_ok(self): ) 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 From 2d5fe2ba2e4384e026f2775e3e299577e01e0115 Mon Sep 17 00:00:00 2001 From: dantp-ai <1534513+dantp-ai@users.noreply.github.com> Date: Wed, 10 Jun 2026 23:30:01 +0200 Subject: [PATCH 4/4] refactor: tighten dry-run wiring post-review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review follow-ups on the dry-run code added by the prior two commits. No behavior change for any existing test (56 still pass). - Move ENVS_USING_MAKE_LLM_CLIENT into clawloop/train.py next to ENV_BUILDERS, where the knowledge actually lives. Add a maintenance note: when registering a new builder, decide whether it routes through _make_llm_client and add the env_type if so; otherwise --dry-run falls back to _StubAdapter. Eliminates cross-module information leakage (cli.py asserting a property of train.py). - Collapse _make_stub_env_builder + _stub_episode into _StubAdapter methods. The factory was used in exactly one place; the helper had one caller. Two entities instead of three; depth unchanged. - Stub task count now tracks max(1, config.episodes_per_iter) instead of a magic 3, so --dry-run matches what the learning loop wants. - Drop default= args from validate_config._positive_int calls; TauBenchAdapter.setup already owns those defaults. Validation now only checks values the user supplied — removes duplicated knowledge and simplifies the helper signature. Co-Authored-By: Claude Opus 4.7 --- clawloop/cli.py | 92 +++++++++++++++++++++-------------------------- clawloop/train.py | 24 ++++++++++--- 2 files changed, 59 insertions(+), 57 deletions(-) diff --git a/clawloop/cli.py b/clawloop/cli.py index c62ff2af..782157e3 100644 --- a/clawloop/cli.py +++ b/clawloop/cli.py @@ -161,14 +161,6 @@ def cmd_run(args: argparse.Namespace) -> None: train(config) -# Envs whose adapter routes its task LLM through `_make_llm_client`. For -# every other env_type the adapter constructs / drives LLMs internally -# (e.g. tau2 inside taubench, EntropicAdapter.setup), so patching -# `_make_llm_client` alone wouldn't stop real network calls — we also have -# to install a stub builder. -_ENVS_USING_MAKE_LLM_CLIENT = frozenset({"math"}) - - def _install_dry_run_clients(config: Any) -> None: """Wire `--dry-run`: guarantee no real LLM calls regardless of env_type. @@ -179,9 +171,9 @@ def _install_dry_run_clients(config: Any) -> None: — 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``, swap the - registered builder with a stub that returns a no-I/O adapter - whose ``run_episode`` / ``run_batch`` produce canned episodes. + 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 @@ -207,29 +199,29 @@ def _mock_make(cfg): _train._make_llm_client = _mock_make # Part 2: for env_types that bypass _make_llm_client, replace the - # registered builder with a stub. Without this, --dry-run on (e.g.) - # taubench / entropic / openclaw would still hit real endpoints. + # 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 - if env_type not in _ENVS_USING_MAKE_LLM_CLIENT and env_type in _train.ENV_BUILDERS: - _train.ENV_BUILDERS[env_type] = _make_stub_env_builder(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, - "stubbed" if env_type not in _ENVS_USING_MAKE_LLM_CLIENT else "uses _make_llm_client", + "uses _make_llm_client" if uses_make_llm_client else "stubbed", ) -def _make_stub_env_builder(env_type: str): - """Return a builder that constructs a no-I/O stub adapter for `env_type`.""" - - def _build_stub(config: Any, llm_clients: Any) -> tuple[Any, list[str]]: - tasks = [f"dry_run_{env_type}_{i}" for i in range(3)] - return _StubAdapter(env_type), tasks - - return _build_stub - - class _StubAdapter: """Adapter that yields canned episodes — no network, no LLM calls. @@ -241,39 +233,35 @@ def __init__(self, env_type: str) -> None: self._env_type = env_type def run_episode(self, task: Any, agent_state: Any) -> Any: - return _stub_episode(self._env_type, task, agent_state) + 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 [_stub_episode(self._env_type, t, agent_state) for t in task_ids] + 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 _stub_episode(env_type: str, 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"{env_type}:{task}", - bench=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 main() -> None: parser = _build_parser() # parse_known_args lets the disabled `eval` subcommand swallow legacy flags diff --git a/clawloop/train.py b/clawloop/train.py index 2c7c560a..d85a329d 100644 --- a/clawloop/train.py +++ b/clawloop/train.py @@ -256,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 @@ -355,10 +366,13 @@ def validate_config(config: TrainConfig) -> list[str]: raise ValueError("taubench env requires 'env_config'") tb = config.env_config - def _positive_int(key: str, default: int | None = None) -> None: - v = tb.get(key, default) - if v is None: + # 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`). @@ -374,8 +388,8 @@ def _positive_int(key: str, default: int | None = None) -> None: raise ValueError(f"taubench env_config.{key} must be a positive int (got {iv})") _positive_int("num_tasks") - _positive_int("max_steps", default=30) - _positive_int("max_concurrency", default=8) + _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