refactor: move dry_run_role off public LLMClientConfig — closes #63

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":
@@ -240,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(

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}
 
 

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,21 +186,47 @@ 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"
 
         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
+
+
+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)}"