fix(eval): apply code review findings

Seven local reviewers and an independent cross-model pass. The headline is that
a fix I added in this branch was worse than the gap it closed.

Reverted the aggregate() quality-median filter. Excluding skill-not-invoked
rows from the quality metrics left valid_runs and excluded_runs still counting
them, so the promotion gate saw N clean runs while the median came from fewer.
The dropped rows are systematically an arm's worst, so it biased toward
PROMOTING - reproduced: one real run at 0.9 plus two uninvoked rows at 0.0 gave
the gate 3 valid runs, zero exclusions and a 0.9 median, flipping keep_incumbent
to promote. Three verdict fields compounded it: they are all() reducers still
reading the wider set, so one uninvoked cell flipped a whole arm. Five
reviewers found the two halves independently.

Closing it honestly needs a scored-run count plus a paired-equality check in
the gate, which is promotion semantics rather than an aggregation fix. The gap
is now pinned by a test that states why the half-fix was reverted.

Stopped forging the absence of CI. The runner refuses --unsafe-no-bwrap when CI
is set because that mode runs sessions with bypassPermissions behind a boundary
its own docstring calls "not a security boundary"; the sweep test deleted CI to
get past it, so eval / locked pytest ran an uncontained agent sweep on the
runner holding the checkout and credentials. It skips under CI instead - the
containment job still runs it for real with GITNEXUS_REQUIRE_FULL_SWEEP=1.

The stand-in CLI was lying in three ways. It never set is_error, so a refused
write read as a completed one. It had no Skill branch at all, so honoring
is_error revealed the evidence gate had been satisfied by a tool the fixture
never ran - the gate was measuring the fixture, not a skill. And a reply with no
usage became four zero-valued fields plus a fabricated cost, which is exactly
the unknown-is-not-zero confusion the accounting it feeds exists to prevent. A
provider failure also crashed the subprocess with no terminal result event.

The identity probe never ran anywhere. test_mock_provider.py was in no job's
file list, and the only job setting CLAUDE_CANARY_BIN runs a fixed list. My
commit message claimed the next containment run would produce the answer; it
would not have. Now wired in, with the CI-shape test updated to pin it.

Also: the regex-miss fallback wrote a predictable name in shared /tmp through a
symlink-following stage, now scoped to the test's own directory; and the
canonical_provider docstring plus the callback comment still asserted a
call_type branch the code no longer has.

Deferred as design decisions rather than review fixes: the containment sweep
uses the stand-in CLI rather than the pinned real one, the full-sweep path
bypasses the gateway so native usage accounting is unexercised there,
_normalize_litellm duplicates the Responses algorithm, and OPENAI_RESPONSES is
now unreachable from canonical_provider.

682 eval tests pass, 17 skipped, ruff clean.

.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

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",

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:

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

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),

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":

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)