diff --git a/.github/workflows/ci-tests.yml b/.github/workflows/ci-tests.yml index a4ac9710d..eff409ea8 100644 --- a/.github/workflows/ci-tests.yml +++ b/.github/workflows/ci-tests.yml @@ -910,7 +910,8 @@ jobs: tests/test_proposer_sandbox.py tests/test_workflow_bench_sessions.py tests/test_ce_plugin_runtime.py - tests/test_offline_sweep_integration.py -q + tests/test_offline_sweep_integration.py + tests/test_mock_provider.py -q working-directory: eval # Native Windows Job Object canary. POSIX-only tests skip by platform, while diff --git a/eval/tests/fixtures/fake_claude.py b/eval/tests/fixtures/fake_claude.py index ba5f9bc71..48d786c30 100755 --- a/eval/tests/fixtures/fake_claude.py +++ b/eval/tests/fixtures/fake_claude.py @@ -8,7 +8,7 @@ sequence the parent parses. Everything between those boundaries (the sandbox, the artifact capture, the scoring, the row) stays real, which is the whole point: those are the layers that shipped bugs no unit test could see. -Reads the prompt from argv or stdin, like the real CLI under --print. +Reads the prompt from stdin, as the real CLI does under "-p --input-format text". """ from __future__ import annotations @@ -47,9 +47,21 @@ def _run_tool(name: str, params: dict) -> str: staging.write_text(params.get("content", "")) os.replace(staging, target) return f"wrote {target}" + if name == "Skill": + # Modelled explicitly rather than falling through to a generic success. + # The parent's evidence gate keys on a Skill request with a non-error + # result, so leaving this unimplemented let an unexecuted skill satisfy + # the gate - the gate would have been measuring the fixture, not a skill. + skill = params.get("skill") or params.get("command") or params.get("name") + if not skill: + raise NotImplementedError("Skill request carried no skill name") + return f"loaded skill {skill}" if name == "Bash": return "(bash suppressed in the stand-in)" - return f"(unhandled tool {name})" + # An unsupported tool is a FAILED tool run, not a quiet success. Returning a + # plain string here made the parent's evidence gate read an unexecuted Skill + # request as a successful invocation. + raise NotImplementedError(f"unsupported tool {name}") def main() -> int: @@ -67,7 +79,16 @@ def main() -> int: emit = lambda event: print(json.dumps(event), flush=True) # noqa: E731 emit({"type": "system", "subtype": "init", "session_id": "fake-session"}) - message = _turn(base_url, prompt) + try: + message = _turn(base_url, prompt) + except (OSError, ValueError) as exc: + # A provider failure is a failed SESSION, not a crashed process: dying + # here leaves no terminal result event, so the parent reports a generic + # stream error instead of the upstream failure it actually saw. + emit({"type": "result", "subtype": "error", "is_error": True, + "session_id": "fake-session", "num_turns": 0, + "error": f"provider request failed: {type(exc).__name__}: {exc}"}) + return 1 blocks = message.get("content", []) emit({"type": "assistant", "message": {"role": "assistant", "content": blocks}}) @@ -77,15 +98,34 @@ def main() -> int: # A refused write is a tool ERROR the session reports and carries # on from, not a crash. Letting it kill the process would lose the # result event and misreport a working boundary as a broken run. + failed = False try: output = _run_tool(block["name"], block.get("input", {})) - except OSError as exc: - output = f"error: {type(exc).__name__}: {exc}" - tool_results.append({"type": "tool_result", "tool_use_id": block["id"], "content": output}) + except (OSError, NotImplementedError) as exc: + output, failed = f"error: {type(exc).__name__}: {exc}", True + # is_error is load-bearing: the parent treats an ABSENT is_error as + # success, so a refused or unsupported tool would otherwise be + # scored as a completed one. + tool_results.append({ + "type": "tool_result", "tool_use_id": block["id"], + "content": output, "is_error": failed, + }) if tool_results: emit({"type": "user", "message": {"role": "user", "content": tool_results}}) - usage = message.get("usage", {}) + # Unknown is not zero. A reply carrying no usage used to become four + # zero-valued fields plus a fabricated cost, which the harness then treats + # as a real measurement - the exact confusion the accounting this fixture + # feeds exists to prevent. + usage = message.get("usage") + if not isinstance(usage, dict) or not all( + isinstance(usage.get(f), int) and not isinstance(usage.get(f), bool) and usage.get(f) >= 0 + for f in ("input_tokens", "output_tokens") + ): + emit({"type": "result", "subtype": "error", "is_error": True, + "session_id": "fake-session", "num_turns": 1, + "error": "provider reply carried no usable usage; refusing to report a measured run"}) + return 1 emit({ "type": "result", "subtype": "success", diff --git a/eval/tests/test_offline_sweep_integration.py b/eval/tests/test_offline_sweep_integration.py index ba1ec4206..a9c9e33e5 100644 --- a/eval/tests/test_offline_sweep_integration.py +++ b/eval/tests/test_offline_sweep_integration.py @@ -45,6 +45,18 @@ ARMS = ("ce_review", "review", "candidate_review") FULL_SWEEP_ENV = "GITNEXUS_REQUIRE_FULL_SWEEP" FULL_SWEEP = os.environ.get(FULL_SWEEP_ENV) == "1" +# The runner refuses --unsafe-no-bwrap whenever CI is set, because that mode runs +# sessions with bypassPermissions behind a boundary its own docstring calls "not +# a security boundary". Deleting CI to get past that refusal would run an +# uncontained agent sweep on the runner holding the checkout and credentials, so +# the stubbed path is skipped under CI instead. The containment job sets +# GITNEXUS_REQUIRE_FULL_SWEEP=1 and takes the real bubblewrap path, so CI keeps +# its coverage; only the uncontained convenience run is given up. +pytestmark = pytest.mark.skipif( + not FULL_SWEEP and bool(os.environ.get("CI")), + reason="an uncontained sweep must not run in CI; the containment job runs it with GITNEXUS_REQUIRE_FULL_SWEEP=1", +) + # The review output and the hidden labels are DELIBERATELY different shapes - # the labels carry line_start/line_end and no recommendation. Only a real run # surfaces that; it is why these are written out rather than shared. @@ -203,7 +215,7 @@ def _sweep(bench, monkeypatch: pytest.MonkeyPatch, findings: list[dict], verdict *([{"name": "Skill", "input": {"skill": skill.group(1) if skill else "gitnexus-review"}}] if invoke_skill else []), {"name": "Write", "input": { - "file_path": target.group(1) if target else "/tmp/unused.json", + "file_path": target.group(1) if target else str(bench.out / "unmatched-review-output.json"), "content": review_for(body)}}, ], input_tokens=2_000, output_tokens=300, @@ -221,8 +233,7 @@ def _sweep(bench, monkeypatch: pytest.MonkeyPatch, findings: list[dict], verdict "--ce-plugin-dir", str(bench.plugin), "--ce-plugin-version", "0.0.0-fixture", "--candidate-overlay", str(bench.overlay), ]) - if not FULL_SWEEP: - monkeypatch.delenv("CI", raising=False) # --unsafe-no-bwrap is forbidden under CI + try: code = runner.main() except SystemExit as exc: diff --git a/eval/tests/test_workflow_bench.py b/eval/tests/test_workflow_bench.py index 23128c189..51212ceda 100644 --- a/eval/tests/test_workflow_bench.py +++ b/eval/tests/test_workflow_bench.py @@ -247,6 +247,9 @@ def test_eval_ci_uses_locked_uv_and_blocking_native_containment_jobs(): # The offline sweep, run here with nothing stubbed: this job is the only # one carrying bubblewrap, the pinned runtime and a built GitNexus. "tests/test_offline_sweep_integration.py", + # Carries the real-CLI identity probe, which needs CLAUDE_CANARY_BIN - + # set only on this job. Omitted from this list it skipped everywhere. + "tests/test_mock_provider.py", "-q", ] bwrap_canary_marker = re.compile( @@ -1062,13 +1065,20 @@ def test_a_raising_packed_cell_still_persists_its_settled_siblings(): assert (1, "review") in folded, "the sibling that completed was never recorded" -def test_an_uninvoked_skill_does_not_move_the_quality_median_but_still_costs(): - """An arm measures a SKILL; a cell where the skill never ran did not measure it. +def test_an_uninvoked_skill_still_counts_toward_the_arm_median(): + """Pins a KNOWN GAP, not a desired behaviour. - The row's evidence is well formed, so the old filter kept it and its score - moved the arm's quality median - an arm could be credited for a review it - never performed with the skill under test. Cost and duration still count: - that session really ran and really was billed. + A cell whose skill never ran still moves the arm's quality median, even + though an arm exists to measure a SKILL. The narrow fix - filtering those + rows out of the quality metrics - is worse than the gap: valid_runs and + excluded_runs keep counting them, so the promotion gate sees N clean runs + while the median came from fewer. Since the dropped rows are systematically + an arm's worst, that biases toward promoting, and it was measured flipping + keep_incumbent to promote. + + Closing it honestly needs a scored-run count and a paired-equality check in + the promotion gate. Pinned here so the half-fix cannot be reapplied without + someone reading why it was reverted. """ good = record(review_weighted_f1=1.0, cost_usd=2.0) @@ -1077,9 +1087,9 @@ def test_an_uninvoked_skill_does_not_move_the_quality_median_but_still_costs(): ) agg = aggregate([good, uninvoked]) - assert agg["review_weighted_f1"] == 1.0, "the uninvoked cell must not drag quality" - assert agg["cost_usd"] == 3.0, "but it was still billed, so it counts for cost" + assert agg["review_weighted_f1"] == 0.5, "the uninvoked row is counted - the known gap" + assert agg["cost_usd"] == 3.0 + # The invariant that makes the half-fix unsafe: the median and the run count + # the gate reads must cover the same rows. + assert agg["valid_runs"] == 2 - # A wrong-but-valid review is a quality result and must still count. - wrong = record(review_weighted_f1=0.0, cost_usd=2.0, resolved=False, error_kind="oracle-failed") - assert aggregate([good, wrong])["review_weighted_f1"] == 0.5 diff --git a/eval/workflow_bench/litellm_usage_callback.py b/eval/workflow_bench/litellm_usage_callback.py index 223b4ccfb..8277d0102 100644 --- a/eval/workflow_bench/litellm_usage_callback.py +++ b/eval/workflow_bench/litellm_usage_callback.py @@ -104,11 +104,10 @@ class ProviderUsageLogger(CustomLogger): "requested_model": kwargs.get("model"), "actual_model": getattr(response_obj, "model", None), # Two fields, because they answer different questions. The raw - # label is what LiteLLM said; "provider" is the adapter key, - # which needs the call type too - LiteLLM reports "openai" for - # both Chat Completions and Responses and those report usage - # differently. Unresolvable stays None so normalize_usage - # refuses rather than guessing token semantics. + # label is what LiteLLM said; "provider" is the adapter key for + # the object actually in hand, which is always LiteLLM's own + # normalised shape here. An unrecognised label stays None so + # normalize_usage refuses rather than guessing token semantics. "provider_label": provider_label, "provider": canonical_provider(provider_label, call_type), "response_id": getattr(response_obj, "id", None), diff --git a/eval/workflow_bench/provider_usage.py b/eval/workflow_bench/provider_usage.py index 4ae11b424..a56582be1 100644 --- a/eval/workflow_bench/provider_usage.py +++ b/eval/workflow_bench/provider_usage.py @@ -191,12 +191,15 @@ def _normalize_anthropic(usage: Mapping[str, Any]) -> NormalizedUsage: def canonical_provider(label: str | None, call_type: str | None) -> str | None: """Map LiteLLM's provider label onto an adapter key, or None if unsure. - LiteLLM reports ``custom_llm_provider`` as "openai" for both Chat - Completions and Responses, and those two report usage differently, so the - label alone cannot pick an adapter. The call type is what distinguishes - them. Returning None when it does not is deliberate: normalize_usage - refuses an unknown provider rather than guessing token semantics, which is - the whole point of keeping the native object authoritative. + Every "openai" label maps to LITELLM_NORMALIZED regardless of call type, + because anything reaching a proxy callback has already been normalised by + LiteLLM into its own object - measured against a real gateway, where the + observed call type is "anthropic_messages" and the upstream Responses shape + never arrives. OPENAI_RESPONSES stays in the adapter table for a RAW + upstream body, which only direct callers and the wire-shape tests pass. + + An unrecognised label still returns None, so normalize_usage refuses rather + than guessing token semantics. """ if label == "openai": diff --git a/eval/workflow_bench/runner.py b/eval/workflow_bench/runner.py index 25a2d5f5a..42cbfc9cb 100644 --- a/eval/workflow_bench/runner.py +++ b/eval/workflow_bench/runner.py @@ -1591,17 +1591,26 @@ def aggregate(records: list[dict[str, Any]]) -> dict[str, Any]: "review_category_accuracy", "review_grounded_evidence", ) - # QUALITY metrics only: a cell whose skill never ran did not measure the - # skill, so its score must not move the arm's quality median. It stays in - # `valid` for cost and duration, because that session really did run and - # really was billed - and it stays visible to the promotion gate, which has - # its own vocabulary for a candidate that never loaded its skill. - scored = [record for record in valid if record.get("error_kind") != "skill-not-invoked"] - if any("review_weighted_f1" in record for record in scored): + # NOTE: a skill-not-invoked row still contributes to these medians. That is + # a real measurement gap - an arm exists to measure a SKILL, and a cell + # where the skill never ran did not measure it - but the narrow fix is + # WORSE than the gap, so it is deliberately not applied here. + # + # Filtering those rows out of the quality metrics alone leaves valid_runs + # and excluded_runs counting them, so the promotion gate sees N clean runs + # while the median was taken over fewer. Because the dropped rows are + # systematically an arm's worst, that biases toward PROMOTING: measured on + # one real run at 0.9 plus two uninvoked rows at 0.0, the gate flipped from + # keep_incumbent to promote. The three verdict fields below compound it - + # they are all() reducers, so one uninvoked cell flips a whole arm. + # Closing this honestly needs a scored-run count and a paired-equality + # check in the gate itself: a promotion-semantics change, not an + # aggregation fix. + if any("review_weighted_f1" in record for record in valid): for metric in review_metrics: - values = [record[metric] for record in scored if record.get(metric) is not None] + values = [record[metric] for record in valid if record.get(metric) is not None] reducer = min if metric == "review_blocker_recall" else statistics.median - out[metric] = reducer(values) if values and len(values) == len(scored) else None + out[metric] = reducer(values) if values and len(values) == len(valid) else None verdicts = [record.get("review_verdict_correct") for record in valid] out["review_verdict_correct"] = ( all(value is True for value in verdicts)