From d901fc04a01b2d9024bd4777d6b0571e26ca7ab4 Mon Sep 17 00:00:00 2001 From: dantp-ai <1534513+dantp-ai@users.noreply.github.com> Date: Thu, 11 Jun 2026 01:35:00 +0200 Subject: [PATCH 1/2] =?UTF-8?q?refactor:=20move=20dry=5Frun=5Frole=20off?= =?UTF-8?q?=20public=20LLMClientConfig=20=E2=80=94=20closes=20#63?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the public ``dry_run_role: str | None = None`` field on ``LLMClientConfig`` (introduced in PR #62) with a private subclass ``_DryRunLLMClientConfig`` defined in ``clawloop/cli.py``. Why a subclass instead of the field: - The public ``LLMClientConfig`` schema no longer carries any dry-run testing vocabulary. ``model_dump()`` / ``model_json_schema()`` of a user's TrainConfig do not emit ``dry_run_role: null`` anymore. New test ``test_public_llm_client_config_schema_excludes_dry_run_vocab`` locks this in. - ``model`` is still left untouched (the gain from PR #62 stands). - Pydantic ``model_copy()`` preserves the runtime class, so the role tag survives copies just like a field would. The regression test is updated to assert this end-to-end. Implementation note: the issue originally proposed a ``WeakKeyDictionary`` side-channel. Empirically, Pydantic v2 BaseModel instances are not hashable by default, so that approach is infeasible without making ``LLMClientConfig`` ``frozen=True`` (which would be a more invasive public-API change). The subclass approach satisfies the same spirit ("keep dry-run vocabulary out of the public schema") via a standard Pydantic mechanism. Verification: pre-commit clean; 57 tests pass; smoke runs for math and taubench dry-run still complete cleanly with no API keys set. Co-Authored-By: Claude Opus 4.7 --- clawloop/cli.py | 47 ++++++++++++++++++++++++++--------- clawloop/train.py | 5 ---- tests/test_cli.py | 62 +++++++++++++++++++++++++++++++++++------------ 3 files changed, 81 insertions(+), 33 deletions(-) diff --git a/clawloop/cli.py b/clawloop/cli.py index 782157e3..429cb42a 100644 --- a/clawloop/cli.py +++ b/clawloop/cli.py @@ -18,8 +18,32 @@ from pathlib import Path from typing import Any +from clawloop.train import LLMClientConfig + log = logging.getLogger("clawloop") + +class _DryRunLLMClientConfig(LLMClientConfig): + """Private subclass that tags an LLM client config with its dry-run role. + + Used only by ``cmd_run --dry-run``: ``_install_dry_run_clients`` swaps + each entry in ``config.llm_clients`` for an instance of this class, and + the patched ``_make_llm_client`` picks the right mock via ``isinstance``. + + Why a subclass instead of a field on ``LLMClientConfig``: + - The public ``LLMClientConfig`` schema stays free of testing + vocabulary — no ``dry_run_role: null`` in JSON dumps or generated + JSON Schema. + - ``model`` is left untouched, so downstream code that reads it + verbatim (e.g. ``_build_entropic`` propagating ``tc.model`` into + ``entropic_cfg``) is unaffected. + - Pydantic ``model_copy()`` preserves the runtime class, so the tag + survives copies just like a field would. + """ + + dry_run_role: str + + _EVAL_DISABLED_MSG = ( "`clawloop eval` is disabled. Use one of:\n" " - Real benchmark: uv run clawloop run examples/configs/math_harness.json\n" @@ -165,12 +189,11 @@ 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. + 1. Replace each ``LLMClientConfig`` in ``config.llm_clients`` with a + ``_DryRunLLMClientConfig`` instance carrying the role, and patch + ``clawloop.train._make_llm_client`` to switch on ``isinstance``. + The private subclass keeps the public schema clean and survives + ``model_copy()``. 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``. @@ -179,16 +202,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: 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 + # Part 1: replace each cfg with a subclass instance carrying the role, + # then route _make_llm_client through a mock factory that reads the role. + for role, cfg in list(config.llm_clients.items()): + config.llm_clients[role] = _DryRunLLMClientConfig(**cfg.model_dump(), dry_run_role=role) original_make = _train._make_llm_client def _mock_make(cfg): - role = getattr(cfg, "dry_run_role", None) - if role is not None: + if isinstance(cfg, _DryRunLLMClientConfig): + role = cfg.dry_run_role if role == "reflector": return MockLLMClient(responses=_build_mock_reflector_responses()) if role == "task": diff --git a/clawloop/train.py b/clawloop/train.py index d85a329d..c1594fbd 100644 --- a/clawloop/train.py +++ b/clawloop/train.py @@ -36,11 +36,6 @@ 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} diff --git a/tests/test_cli.py b/tests/test_cli.py index ac4ef9cb..ba2d4332 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -154,15 +154,19 @@ def test_run_openclaw_dry_run_no_api_calls(tmp_path: Path, monkeypatch): # --------------------------------------------------------------------------- -# Role marker survives Pydantic copy (PR #60 review comment 2) +# Dry-run role tagging (PR #60 comment 2; PR #62 comment 1; issue #63) # --------------------------------------------------------------------------- -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.""" +def test_dry_run_subclass_survives_pydantic_copy(monkeypatch): + """`_install_dry_run_clients` replaces each entry in `config.llm_clients` + with a `_DryRunLLMClientConfig` instance. The subclass: + * survives `model_copy()` (Pydantic preserves runtime class), + * leaves `model` untouched so downstream consumers see it verbatim, + * is detected via `isinstance` by the patched `_make_llm_client`. + + Regression for issue #63: ensures the role does not live on the public + `LLMClientConfig` schema.""" import clawloop.cli as _cli import clawloop.train as _train from clawloop.demo_math import MockTaskClient @@ -182,17 +186,24 @@ def test_dry_run_role_field_survives_pydantic_copy(monkeypatch): _cli._install_dry_run_clients(cfg) try: + reflector = cfg.llm_clients["reflector"] + task = cfg.llm_clients["task"] + + # Each entry is now a _DryRunLLMClientConfig subclass instance. + assert isinstance(reflector, _cli._DryRunLLMClientConfig) + assert isinstance(task, _cli._DryRunLLMClientConfig) # `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 reflector.model == "anthropic/claude-sonnet-4" + assert task.model == "anthropic/claude-haiku-4" + # The role lives on the subclass instance. + assert reflector.dry_run_role == "reflector" + assert task.dry_run_role == "task" + + # `model_copy()` preserves the runtime class so the tag survives. + copied_reflector = reflector.model_copy() + copied_task = task.model_copy() + assert id(copied_reflector) != id(reflector) + assert isinstance(copied_reflector, _cli._DryRunLLMClientConfig) assert copied_reflector.dry_run_role == "reflector" assert copied_task.dry_run_role == "task" @@ -200,3 +211,22 @@ def test_dry_run_role_field_survives_pydantic_copy(monkeypatch): assert isinstance(_train._make_llm_client(copied_task), MockTaskClient) finally: _train._make_llm_client = original_make + + +def test_public_llm_client_config_schema_excludes_dry_run_vocab(): + """The public `LLMClientConfig` schema must not carry any dry-run + testing vocabulary — that lives on the private `_DryRunLLMClientConfig` + subclass instead. Acceptance criterion for issue #63.""" + from clawloop.train import LLMClientConfig + + schema = LLMClientConfig.model_json_schema() + properties = set(schema.get("properties", {}).keys()) + assert ( + "dry_run_role" not in properties + ), f"LLMClientConfig should not expose dry_run_role; got {sorted(properties)}" + + # And model_dump of a vanilla instance must not emit it either. + dumped = LLMClientConfig(model="anthropic/x").model_dump() + assert ( + "dry_run_role" not in dumped + ), f"vanilla LLMClientConfig.model_dump should not include dry_run_role; got keys {sorted(dumped)}" From f5e75444fdc2c01284dd05fa515da50f10af3ce9 Mon Sep 17 00:00:00 2001 From: dantp-ai <1534513+dantp-ai@users.noreply.github.com> Date: Thu, 11 Jun 2026 01:39:48 +0200 Subject: [PATCH 2/2] fix: tighten exception in _StubAdapter.run_episode per PR #65 review `except Exception:` swallowed any error from `agent_state.state_id().combined_hash`. For the dry-run stub the only legitimate failure modes are AttributeError (missing `state_id` / `combined_hash`) and TypeError (non-callable). Narrow the catch so real bugs propagate instead of being silently masked. Co-Authored-By: Claude Opus 4.7 --- clawloop/cli.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/clawloop/cli.py b/clawloop/cli.py index 429cb42a..2e7bd8e3 100644 --- a/clawloop/cli.py +++ b/clawloop/cli.py @@ -263,7 +263,10 @@ def run_episode(self, task: Any, agent_state: Any) -> Any: state_id = "" try: state_id = agent_state.state_id().combined_hash - except Exception: + except (AttributeError, TypeError): + # AttributeError: agent_state has no `state_id` or the result + # lacks `combined_hash`. TypeError: `state_id` is not callable. + # Any other exception is a real bug and should propagate. pass return Episode(