From b212f9c6bf37dc521520d4e61b58190f00c6bf23 Mon Sep 17 00:00:00 2001 From: Trevor Hudson Date: Thu, 30 Jul 2026 16:11:18 -0700 Subject: [PATCH] Close six more test gaps found by a second mutation sweep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first sweep covered the adapters. This one covers what it didn't: the compiler drivers, the inference helper, the MCP runtime helper, and probe — the modules where this project's expensive bugs have actually lived. 17 mutations, each reproducing a real shipped bug or an explicit CLAUDE.md guard; 11 caught on the first pass. **The agent-loop CLI path is the weak twin of the judge path.** `_runtime_helper` carries two copies of the `claude -p` flag block, and mutations died on the judge copy while surviving on the loop copy: `--setting-sources` and `is_error` were covered on one and not the other, and `--system-prompt` on neither. That is the same asymmetry that let the bare `--tools` flag ship broken in 0.11.0 — the loop path had no CLI coverage at all. Four new tests bring the loop up to parity: - `--system-prompt` carries a short custom prompt on BOTH paths. Dropping it silently restores Claude Code's whole coding-agent prompt: ~37k cache-creation tokens and 11.5s for a one-sentence judge, versus ~740 and ~3.5s. Asserted as "non-empty and under 400 chars" — presence alone would pass with an empty value, and exact text would break on any rewording. - `--setting-sources ""` on the loop. Without it an emitted pipeline inherits whatever hooks, MCP servers and permissions the developer's machine has, so it behaves differently per laptop — the opposite of what compiling a skill is for. - `is_error` on the loop. `claude -p` reports API failures inside a ZERO exit, so returncode alone calls a dead loop a win and the workflow proceeds on garbage. **A symbolic assertion cannot pin a constant.** The driver tests said `args[args.index("--model") + 1] == DEFAULT_MODEL`, which verifies the flag is wired up and says nothing about the constant — both sides move together. So `DEFAULT_MODEL` could be flipped to Opus (~5x cost; two Opus runs of BDR exhausted a Max extra-usage budget) and `DEFAULT_MAX_TURNS` halved (the original `error_max_turns` failure) with the suite fully green, despite CLAUDE.md documenting both as do-not-change. `tests/test_cost_defaults.py` pins the *property* — not-Opus, `>= 60`, and the two drivers agreeing — so bumping the Sonnet generation stays a one-line change while the costly regression fails. **A test that never ran.** `test_per_tool_override_changes_payload` took its mcp-bound node from the BDR fixture and skipped when there wasn't one — and there never was: BDR's five required MCP servers all come from its two agent loops' `tool_servers`, while its four external_call nodes carry no `mcp:` binding. It now builds its own two-node fixture and asserts the exact split (one pinned tool at the override, one at the default), so a leak onto every external_call fails too. The suite has no skips left. `-rs` added to pytest addopts so skip REASONS always print. A conditional skip whose condition quietly becomes permanent is invisible behind a bare "1 skipped", which is how the above went from a real test to dead weight unnoticed. Every fix verified by re-running the mutation it was written for: all six previously-surviving mutations are now caught. 1170 fast + 27 slow pass; ruff, strict mypy, sanity-check clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SBuSDL7Svm7iDzgmhTpDSW --- CLAUDE.md | 39 +++++++++--- pyproject.toml | 6 +- tests/test_cost_defaults.py | 79 ++++++++++++++++++++++++ tests/test_eval_estimate.py | 54 ++++++++++++++--- tests/test_inference.py | 117 ++++++++++++++++++++++++++++++++++++ 5 files changed, 277 insertions(+), 18 deletions(-) create mode 100644 tests/test_cost_defaults.py diff --git a/CLAUDE.md b/CLAUDE.md index 37cd812..324cc4b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -834,13 +834,38 @@ already worked, proving it recognizes correct behavior rather than just matching new strings. A periodic mutation sweep is the systematic version, and it works well -here because emission is pure template substitution — mutate an adapter, +here because emission is pure template substitution — mutate a module, run `pytest tests/`, and any mutation that survives is an untested -behavior. The sweep that produced the table above ran 18 mutations -(retry policy, timeouts, step naming, payload threading, MCP allowlist -narrowing, the `ANTHROPIC_API_KEY` scrub, invariant #4 and #7) and -caught 15. Watch for **equivalent mutants** — the fan_out tier ordering -in `fan_out_element_param` survives because it is provably unobservable, +behavior. Two sweeps so far, 35 mutations, 26 caught on the first pass: + +- **Adapters** (18): retry policy, timeouts, step naming, payload + threading, MCP allowlist narrowing, the `ANTHROPIC_API_KEY` scrub, + invariants #4 and #7. Gaps found: per-node `timeout:` was ignorable on + both temporal and cloudflare gates with nothing failing. +- **Drivers / inference / MCP / probe** (17): the flags that keep + `claude -p` cheap, `is_error` inside a zero exit, fastmcp's + `result.data`, probe's never-guess rule, and the cost-critical driver + defaults. + +Two lessons from the second sweep worth keeping: + +- **The agent-loop CLI path is the weak twin.** `_runtime_helper` carries + two copies of the `claude -p` flag block (judge, agent loop), and + mutations died on the judge copy while surviving on the loop copy — + `--setting-sources` and `is_error` were covered on one and not the + other, and `--system-prompt` on neither. That asymmetry is exactly how + the bare `--tools` bug shipped. When you touch one copy, test both. +- **A symbolic assertion cannot pin a constant.** + `args[args.index("--model") + 1] == DEFAULT_MODEL` verifies the flag is + wired up and says nothing about the constant, because both sides move + together. `DEFAULT_MODEL` could be flipped to Opus (~5x cost) and + `DEFAULT_MAX_TURNS` halved (the original `error_max_turns` failure) + with the suite fully green. `tests/test_cost_defaults.py` pins the + *property* — not-Opus, `>= 60` — so a legitimate model bump passes and + the expensive regression fails. + +Watch for **equivalent mutants** — the fan_out tier ordering in +`fan_out_element_param` survives because it is provably unobservable, not because it is untested; that one is commented in place. --- @@ -1139,7 +1164,7 @@ Don't waste time debugging stubs. These are intentional. pipeline with cross-process gate signaling via `DBOSClient`; real judge usage captured through the emitted `$ROTE_USAGE_LOG` hook; measurements appended to `~/.local/share/rote/eval-corpus.jsonl`) -- 1189 tests (1162 fast + 27 slow). Run with `pytest tests/` (fast +- 1197 tests (1170 fast + 27 slow). Run with `pytest tests/` (fast only — what runs by default). Slow tests cover the runtime e2e suites (Temporal, Cloudflare, DBOS, DBOS-TS, Inngest, MCP-over-stdio); the TS ones require a Node toolchain, DBOS-TS diff --git a/pyproject.toml b/pyproject.toml index fac4cc2..4cfe244 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -138,7 +138,11 @@ extend-exclude = ["examples"] select = ["E", "F", "I", "UP", "B", "SIM"] [tool.pytest.ini_options] -addopts = "-m 'not slow'" +# -rs: always print the REASON for every skip. A conditional skip whose +# condition silently becomes permanent is invisible behind a bare +# "1 skipped" count — that is how `test_per_tool_override_changes_payload` +# went from a real test to dead weight without anyone noticing. +addopts = "-m 'not slow' -rs" markers = [ "slow: tests that need an external toolchain (Node/npm/tsc) or take >5s. Run with `pytest -m slow` to include.", ] diff --git a/tests/test_cost_defaults.py b/tests/test_cost_defaults.py new file mode 100644 index 0000000..919eaee --- /dev/null +++ b/tests/test_cost_defaults.py @@ -0,0 +1,79 @@ +"""Defaults that cost real money when they change. + +Two compiler-driver constants have burned this project before, and both +are documented in CLAUDE.md as "don't change this" — but documentation +is not a test. A mutation sweep confirmed it: flipping the default model +to Opus and halving the turn budget both left the entire suite green. + +The existing driver tests compare against the constants *symbolically* +(``args[args.index("--model") + 1] == DEFAULT_MODEL``), which is right +for checking the flag is wired up and useless for catching a change to +the constant itself — both sides move together. + +These tests pin the *property* rather than the literal, so a legitimate +upgrade (sonnet-4-6 → sonnet-5) passes while the expensive regression +fails. +""" + +from __future__ import annotations + +import re + +from rote.compiler.drivers.anthropic_api import DEFAULT_MODEL as API_DEFAULT_MODEL +from rote.compiler.drivers.claude import DEFAULT_MAX_TURNS +from rote.compiler.drivers.claude import DEFAULT_MODEL as CLI_DEFAULT_MODEL + +#: Compiling BDR ran ~$3.50 per attempt on Opus and exhausted a Claude +#: Max "extra usage" budget in two runs. Sonnet follows the structured +#: rubric perfectly well; Opus is ~5x the price for no measured gain. +_OPUS_RE = re.compile(r"opus", re.IGNORECASE) + +#: BDR-scale skills need ~25 tool calls minimum and realistically 40-50 +#: with exploration. The original default of 30 produced a hard +#: `error_max_turns` failure on the first real run. +_MIN_MAX_TURNS = 60 + + +def test_compiler_drivers_do_not_default_to_opus() -> None: + """Both drivers default to a non-Opus model. + + Asserted as "not Opus" rather than an exact model id so bumping the + Sonnet generation stays a one-line change, while the regression this + exists to prevent still fails. + """ + for name, model in ( + ("ClaudeDriver", CLI_DEFAULT_MODEL), + ("AnthropicApiDriver", API_DEFAULT_MODEL), + ): + assert not _OPUS_RE.search(model), ( + f"{name}.DEFAULT_MODEL is {model!r}. Opus is ~5x Sonnet's price " + f"and Sonnet follows the compiler rubric fine — two Opus runs of " + f"BDR exhausted a Max extra-usage budget. If you have specific " + f"evidence a skill needs Opus, pass model= explicitly rather than " + f"changing the default for everyone." + ) + + +def test_both_drivers_share_a_default_model() -> None: + """The subprocess and in-process drivers must not drift apart. + + They are two ways to run the same compiler agent; a different default + on each makes compile cost depend on which driver happened to be + selected, which is invisible to the user. + """ + assert CLI_DEFAULT_MODEL == API_DEFAULT_MODEL + + +def test_max_turns_leaves_headroom_for_bdr_scale_skills() -> None: + """The turn budget stays >= 60. + + Not an equality check: raising it is harmless, lowering it silently + truncates a compile into `error_max_turns` after the run has already + spent most of its money. + """ + assert DEFAULT_MAX_TURNS >= _MIN_MAX_TURNS, ( + f"DEFAULT_MAX_TURNS is {DEFAULT_MAX_TURNS}. BDR-scale skills need " + f"~25 tool calls minimum and 40-50 with exploration; the original " + f"default of 30 caused a hard error_max_turns failure on the first " + f"real run. Don't reduce it without measuring." + ) diff --git a/tests/test_eval_estimate.py b/tests/test_eval_estimate.py index f17c813..aab3511 100644 --- a/tests/test_eval_estimate.py +++ b/tests/test_eval_estimate.py @@ -19,7 +19,7 @@ from rote.eval.priors import Priors from rote.eval.sidecar import EvalEstimates, StepEstimate, TurnRange from rote.eval.tokens import HeuristicTokenCounter -from rote.ir import NodeKind, Pipeline, load_pipeline +from rote.ir import Node, NodeKind, Pipeline, load_pipeline REPO_ROOT = Path(__file__).resolve().parent.parent BDR_PIPELINE = REPO_ROOT / "examples" / "bdr-outreach" / "expected" / "pipeline.yaml" @@ -312,19 +312,53 @@ def test_external_call_payload_sums_footprint(bdr_pipeline: Pipeline) -> None: assert payload == n_external * priors.tokens_per_external_call_result -def test_per_tool_override_changes_payload(bdr_pipeline: Pipeline) -> None: - """A pinned per-tool payload beats the default constant for that tool.""" +def test_per_tool_override_changes_payload() -> None: + """A pinned per-tool payload beats the default constant for that tool. + + Built on a purpose-made fixture rather than the BDR example. This + test previously took its mcp-bound node from BDR and skipped when + there wasn't one — and there never was: BDR's five required MCP + servers all come from its two agent loops' `tool_servers`, while its + four external_call nodes carry no `mcp:` binding at all. So it + silently never ran. A test whose subject is an example's incidental + shape is a test that can quietly stop testing. + """ from rote.eval.estimate import external_call_payload_tokens - external = [n for n in bdr_pipeline.nodes if n.kind is NodeKind.EXTERNAL_CALL and n.mcp] - if not external: - pytest.skip("BDR fixture has no MCP-bound external_call to override") - tool = external[0].mcp.tool - base = external_call_payload_tokens(bdr_pipeline, Priors()) + pinned = Node( + id="fetch_pinned", + kind=NodeKind.EXTERNAL_CALL, + description="d", + impl="m.py:fetch", + mcp={"server": "slack", "tool": "slack_get_messages"}, + ) + unpinned = Node( + id="fetch_other", + kind=NodeKind.EXTERNAL_CALL, + description="d", + impl="m.py:other", + mcp={"server": "gmail", "tool": "gmail_search"}, + ) + pipeline = Pipeline( + name="payloads", + input={"type": "In", "required": [], "optional": []}, + nodes=[pinned, unpinned], + edges=[{"from": "fetch_pinned", "to": "fetch_other"}], + entry_nodes=["fetch_pinned"], + exit_nodes=["fetch_other"], + ) + + priors = Priors() + default_each = priors.tokens_per_external_call_result + assert external_call_payload_tokens(pipeline, priors) == 2 * default_each + bumped = external_call_payload_tokens( - bdr_pipeline, Priors(payload_tokens_per_tool={tool: 99_000.0}) + pipeline, Priors(payload_tokens_per_tool={"slack_get_messages": 99_000.0}) ) - assert bumped > base + # Exactly one node is pinned: the other keeps the default. Asserting + # `bumped > base` alone would also pass if the override leaked onto + # every external_call. + assert bumped == 99_000.0 + default_each def test_priors_from_overrides_scalars_and_per_tool() -> None: diff --git a/tests/test_inference.py b/tests/test_inference.py index 1af51ca..f29ff84 100644 --- a/tests/test_inference.py +++ b/tests/test_inference.py @@ -633,3 +633,120 @@ def resolve_url(name: str, _pipeline_url: None) -> str: # better to go on, and a name no server provides never resolves anyway. _servers, unpinned = helper._mcp_config_for(tools, None) assert len(unpinned) == 4 + + +# ───────── The overhead-control flags, on BOTH CLI paths ───────── +# +# `claude -p` defaults to shipping Claude Code's entire coding-agent +# system prompt: ~37k cache-creation tokens and 11.5s for a one-sentence +# judge. `--system-prompt ` + `--tools ""` + `--setting-sources +# ""` bring the identical call to ~740 tokens and ~3.5s. +# +# A mutation sweep found `--system-prompt` untested on BOTH paths and +# `--setting-sources` / `is_error` untested on the agent-loop path — the +# same asymmetry that let the bare `--tools` flag ship broken, since the +# agent-loop CLI path had no coverage at all. + + +def _cli_flag(command: list[str], flag: str) -> str: + assert flag in command, f"{flag} missing from the invocation" + return command[command.index(flag) + 1] + + +def test_judge_cli_replaces_the_default_coding_agent_prompt( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A judge must carry its own short system prompt. + + Asserting presence would pass with an empty value; asserting the + exact text would break on any wording change. The load-bearing + property is that it is *short and custom* — dropping the flag is + what restores the 37k-token default. + """ + monkeypatch.setattr(helper, "_mcp_config_for", lambda *_a: ({}, [])) + seen = _capture_cli(monkeypatch, _cli_envelope(structured_output={"grade": 9})) + + helper.call_judge( + node_id="grade_essay", + client="anthropic", + prompt="Grade it.", + output_schema={"type": "object", "properties": {"grade": {"type": "integer"}}}, + model="claude-sonnet-4-6", + ) + + prompt = _cli_flag(seen["command"], "--system-prompt") + assert prompt.strip(), "an empty system prompt restores Claude Code's default" + assert len(prompt) < 400, ( + f"the judge system prompt is {len(prompt)} chars — it is meant to be a " + f"one-liner; the whole point of the flag is not shipping a large prompt" + ) + + +def test_agent_loop_cli_replaces_the_default_coding_agent_prompt( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Same contract on the agent-loop path, which had no coverage of it.""" + monkeypatch.setattr(helper, "_mcp_config_for", lambda *_a: ({}, [])) + seen = _capture_cli(monkeypatch, _agent_envelope()) + + helper.run_agent_loop( + node_id="target_research", + description="Research the account.", + task='{"account": "acme"}', + model="claude-sonnet-4-6", + tools=["bright_data_search"], + max_iterations=6, + ) + + prompt = _cli_flag(seen["command"], "--system-prompt") + assert prompt.strip(), "an empty system prompt restores Claude Code's default" + assert len(prompt) < 400 + + +def test_agent_loop_cli_does_not_inherit_user_settings( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """`--setting-sources ""` on the agent loop, as on the judge. + + Without it the loop picks up whatever hooks, MCP servers and + permissions the *developer's* machine has configured — so an emitted + pipeline behaves differently per laptop, which is the opposite of + what compiling a skill is for. + """ + monkeypatch.setattr(helper, "_mcp_config_for", lambda *_a: ({}, [])) + seen = _capture_cli(monkeypatch, _agent_envelope()) + + helper.run_agent_loop( + node_id="target_research", + description="Research the account.", + task='{"account": "acme"}', + model="claude-sonnet-4-6", + tools=["bright_data_search"], + max_iterations=6, + ) + + assert _cli_flag(seen["command"], "--setting-sources") == "" + + +def test_agent_loop_cli_surfaces_the_envelopes_error( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A failed agent loop must not read as a success. + + `claude -p` reports API failures *inside a zero exit* — `is_error` + in the envelope — so returncode alone calls a dead loop a win and + the workflow proceeds on a garbage result. Covered on the judge + path; this is the loop half. + """ + monkeypatch.setattr(helper, "_mcp_config_for", lambda *_a: ({}, [])) + _capture_cli(monkeypatch, _agent_envelope(is_error=True, result="rate limited")) + + with pytest.raises(RuntimeError, match="rate limited"): + helper.run_agent_loop( + node_id="target_research", + description="Research the account.", + task='{"account": "acme"}', + model="claude-sonnet-4-6", + tools=["bright_data_search"], + max_iterations=6, + )