From c44beccb389fb6c9d2282f106837b076a4472cbe Mon Sep 17 00:00:00 2001 From: Trevor Hudson Date: Thu, 30 Jul 2026 17:33:22 -0700 Subject: [PATCH] Close six test gaps found by a third mutation sweep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Swept rote.eval, rote.config and rote.cli — 29 wrong-but-plausible mutations, 22 caught, 6 real gaps, 1 equivalent mutant. Two areas were already airtight and needed nothing: the four-layer config precedence (flag > env > project > user > built-in, every reorder died) and pricing's current-generation tier detection. The gaps clustered where a wrong answer looks like a right one. Dollar arithmetic. `pipeline_cost_usd` recomputes an expected figure from the fixture rates, so every mutation to it died. `agent_run_cost_usd` was only ever compared against itself — `with_cache.mid < without_cache.mid` — and an ordering is invariant under any transformation applied to both sides. The divisor could become 1e3 instead of 1e6, or the cache-write premium could disappear, and the suite stayed green. That number is what a user makes a spending decision on, so it is now pinned to the dollars, both bounds, with the no-cache-pricing model as the negative half. The agent_loop estimate. It asserted `calls.high` and nothing derived from it, so both the token and the wall-time lines could drop `agent_loop_turns_per_iteration` — a 3x understatement, in exactly the turn-dominated regime the loop-aware model was built for. Config strictness. Four commands call `load_layers()`; only `rote config` asserted exit 2. Compile's handler could `return 0`, reporting success while doing nothing. The new test parametrizes over all four and also asserts nothing was emitted, so the next command to read config fails here rather than shipping a silent fallback. Separately, a blank value survives the emptiness check for `model` — the only free-form key, where the enum check cannot catch it and a blank would reach the driver as a model name. Each fix was verified by re-running its mutation: all six now fail. `args.no_deploy` in _cmd_compile's cloud-vs-local branch is an equivalent mutant — it already forced `deploy_rv.value == "none"` one line above, so removing it is unobservable. Confirmed empirically and commented in place rather than tested. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SBuSDL7Svm7iDzgmhTpDSW --- CLAUDE.md | 40 +++++++++++++++++++++++++--- src/rote/cli.py | 6 +++++ tests/test_config.py | 51 ++++++++++++++++++++++++++++++++++++ tests/test_eval_estimate.py | 43 ++++++++++++++++++++++++++++++ tests/test_eval_scorecard.py | 28 ++++++++++++++++++++ 5 files changed, 164 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b240942..d14eb34 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -881,7 +881,7 @@ matching new strings. A periodic mutation sweep is the systematic version, and it works well here because emission is pure template substitution — mutate a module, run `pytest tests/`, and any mutation that survives is an untested -behavior. Two sweeps so far, 35 mutations, 26 caught on the first pass: +behavior. Three sweeps so far, 64 mutations, 48 caught on the first pass: - **Adapters** (18): retry policy, timeouts, step naming, payload threading, MCP allowlist narrowing, the `ANTHROPIC_API_KEY` scrub, @@ -891,6 +891,12 @@ behavior. Two sweeps so far, 35 mutations, 26 caught on the first pass: `claude -p` cheap, `is_error` inside a zero exit, fastmcp's `result.data`, probe's never-guess rule, and the cost-critical driver defaults. +- **`rote.eval` / `rote.config` / `rote.cli`** (29): dollar arithmetic, + tier detection, the read-only MCP gate, the four-layer config + precedence, and the login-aware compile default. Config precedence and + pricing tier detection were already airtight — every mutation died. + Six gaps found, all in the two places where a wrong number or a wrong + exit code looks like success (see below). Two lessons from the second sweep worth keeping: @@ -909,9 +915,35 @@ Two lessons from the second sweep worth keeping: *property* — not-Opus, `>= 60` — so a legitimate model bump passes and the expensive regression fails. +Three more from the third sweep: + +- **A self-comparison is the same tautology in disguise.** + `pipeline_cost_usd` had `cost.high == pytest.approx(expected_high)` + recomputed from the fixture rates and killed every mutation. + `agent_run_cost_usd` was only ever compared against *itself* + (`with_cache.mid < without_cache.mid`) — an ordering that holds under + any transformation applied to both sides, so the divisor could become + 1e3 instead of 1e6, or the cache-write premium could vanish, with the + suite green. Cost numbers are what a user makes a spending decision + on; assert the dollars. +- **Asserting the loop bound leaves the per-call figures unpinned.** The + `agent_loop` estimate asserted `calls.high` and nothing else, so both + the token and the wall-time lines could drop + `agent_loop_turns_per_iteration` (a 3x understatement) undetected — + in the turn-dominated regime the loop-aware model exists to capture. +- **Strictness is a property of a layer, not of one command.** Four + commands call `load_layers()`; only `rote config` asserted exit 2, so + compile's handler could `return 0` — reporting success while doing + nothing. The replacement test parametrizes over all four, and the + next command to read config fails there instead of shipping a silent + fallback. + 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. +`fan_out_element_param`, and `args.no_deploy` in `_cmd_compile`'s +cloud-vs-local branch, both survive because they are provably +unobservable, not because they are untested. Both are commented in +place; confirm the claim empirically before writing one off, since +"probably equivalent" is exactly what a real gap looks like. --- @@ -1209,7 +1241,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`) -- 1202 tests (1175 fast + 27 slow). Run with `pytest tests/` (fast +- 1214 tests (1187 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/src/rote/cli.py b/src/rote/cli.py index b8446b9..1eabc7b 100644 --- a/src/rote/cli.py +++ b/src/rote/cli.py @@ -449,6 +449,12 @@ def _cmd_compile(args: argparse.Namespace) -> int: if args.cloud: use_cloud = True elif args.local or args.no_deploy or cloud_cred is None: + # `args.no_deploy` here is redundant but deliberate: it already + # forced `deploy_rv.value == "none"` above (it is that resolve's + # flag argument), so `config_prefers_local` covers it. Dropping + # it is an equivalent mutation — unobservable, not untested — and + # it stays because this is where a reader looks to learn what + # keeps a run local. use_cloud = False else: use_cloud = not config_prefers_local diff --git a/tests/test_config.py b/tests/test_config.py index 6d92ac3..4a6995c 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -97,6 +97,21 @@ def test_empty_file_is_fine(tmp_path: Path) -> None: assert load_config_file(path) == {} +@pytest.mark.parametrize("blank", ["", '""', "' '", "\n"]) +def test_a_blank_value_is_rejected_for_a_free_form_key(tmp_path: Path, blank: str) -> None: + """`model` takes any string, so the enum check cannot catch a blank. + + A key with `valid_choices()` rejects "" incidentally (it isn't in + the choices), which is why dropping the emptiness check survived a + mutation sweep — `model` is the only key where it is load-bearing, + and a blank one would reach the driver as a model name. + """ + path = tmp_path / "config.yaml" + path.write_text(f"model: {blank}\n", encoding="utf-8") + with pytest.raises(ConfigError, match="must be a non-empty string"): + load_config_file(path) + + def test_write_config_round_trips(tmp_path: Path) -> None: path = tmp_path / "config.yaml" write_config(path, {"runtime": "temporal", "deploy": "none", "agent": "codex"}) @@ -205,6 +220,42 @@ def test_config_bad_file_exits_2( assert "must be a non-empty string" in capsys.readouterr().err +@pytest.mark.parametrize("command", ["config", "emit", "analyze", "compile"]) +def test_every_config_reading_command_exits_2_on_a_bad_file( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + command: str, +) -> None: + """Strictness is a property of the config layer, not of one command. + + `rote config` was the only command asserting exit 2; flipping + compile's handler to `return 0` — reporting success while doing + nothing — survived a mutation sweep. Every command that calls + `load_layers()` owns this contract, so cover them together and let a + new one fail here rather than shipping a silent fallback. + """ + from tests.conftest import BDR_PIPELINE_YAML + + bad = tmp_path / "config.yaml" + bad.write_text("runtime: clouflare\n", encoding="utf-8") + monkeypatch.setenv("ROTE_CONFIG_PATH", str(bad)) + + skill_dir = _skill(tmp_path) + argv = { + "config": ["config"], + "emit": ["emit", str(BDR_PIPELINE_YAML), "--out", str(tmp_path / "out")], + "analyze": ["analyze", str(skill_dir)], + "compile": ["compile", str(skill_dir), "--out", str(tmp_path / "out"), "--no-eval"], + }[command] + + assert cli_main(argv) == 2 + err = capsys.readouterr().err + assert "expected one of" in err and "clouflare" in err + # The run stopped at the config, before any work: nothing emitted. + assert not (tmp_path / "out").exists() + + # ───────── compile/emit honor the config ───────── diff --git a/tests/test_eval_estimate.py b/tests/test_eval_estimate.py index aab3511..af46a71 100644 --- a/tests/test_eval_estimate.py +++ b/tests/test_eval_estimate.py @@ -134,6 +134,49 @@ def test_unbounded_agent_loop_gets_prior_default_and_note( assert node_est.note is not None and "no termination config" in node_est.note +def test_agent_loop_per_call_cost_scales_with_turns_per_iteration( + counter: HeuristicTokenCounter, +) -> None: + """One loop iteration is several agent turns, and every per-call + figure must carry that factor. + + This is the turn-dominated cost regime the loop-aware model exists + for: `agent_loop_turns_per_iteration` defaults to 3, so dropping it + understates a loop's tokens and wall time threefold. Asserting + `calls` alone leaves all three derived numbers unpinned — a mutation + sweep dropped the factor from both the token and the seconds line + with the suite green. + """ + pipeline = Pipeline.model_validate( + { + "name": "loop-only", + "input": {"type": "In"}, + "nodes": [ + { + "id": "explore", + "kind": "agent_loop", + "description": "bounded exploration", + "tools": ["search"], + "termination": {"max_iterations": 4, "condition": "nothing left to search"}, + } + ], + "edges": [], + } + ) + priors = Priors() + turns = priors.agent_loop_turns_per_iteration + assert turns > 1, "a degenerate 1.0 would make this test unable to fail" + + (node_est,) = estimate_pipeline(pipeline, counter, priors).nodes + assert node_est.calls.high == 4 # the declared bound, not the prior default + assert node_est.note is None + assert node_est.llm_input_tokens_per_call == round(turns * priors.transcript_growth_per_turn) + assert node_est.llm_output_tokens_per_call == round(turns * priors.output_tokens_per_turn) + assert node_est.wall_seconds_per_call == pytest.approx(turns * priors.seconds_per_turn) + # …and the per-call figures compound over the iteration bound. + assert node_est.wall_seconds.high == pytest.approx(4 * turns * priors.seconds_per_turn) + + def test_judge_tokens_scale_with_prompt_and_fields(counter: HeuristicTokenCounter) -> None: def judge_pipeline(prompt: str, out_fields: dict[str, object]) -> Pipeline: return Pipeline.model_validate( diff --git a/tests/test_eval_scorecard.py b/tests/test_eval_scorecard.py index 5c85134..03701b7 100644 --- a/tests/test_eval_scorecard.py +++ b/tests/test_eval_scorecard.py @@ -58,6 +58,34 @@ def estimates(): # type: ignore[no-untyped-def] return pipeline, pe, se +def test_agent_cost_is_the_cache_aware_sum(estimates) -> None: # type: ignore[no-untyped-def] + """Pin the dollars, not just the ordering. + + The comparison test below measures this function against *itself*, + so it stays green when every rate is wrong by the same factor — a + 1000x unit slip, or the cache rates dropped for the plain input + price. Both survived a mutation sweep. The four rates in `_price()` + are distinct on purpose (12.5 write / 1.0 read / 50.0 output / 10.0 + input); only the correct pairing reproduces these numbers. + """ + _, _, se = estimates + cost = agent_run_cost_usd(se, _price()) + for bound in ("low", "high"): + expected = ( + getattr(se.fresh_input_tokens, bound) * 12.5 + + getattr(se.cached_read_tokens, bound) * 1.0 + + getattr(se.output_tokens, bound) * 50.0 + ) / 1_000_000 + assert getattr(cost, bound) == pytest.approx(expected) + # A model with no cache pricing bills both halves at plain input. + plain = agent_run_cost_usd(se, _price(cache_read_per_mtok=None, cache_write_per_mtok=None)) + expected_plain = ( + (se.fresh_input_tokens.high + se.cached_read_tokens.high) * 10.0 + + se.output_tokens.high * 50.0 + ) / 1_000_000 + assert plain.high == pytest.approx(expected_plain) + + def test_agent_cost_is_cache_aware(estimates) -> None: # type: ignore[no-untyped-def] _, _, se = estimates with_cache = agent_run_cost_usd(se, _price())