From 79f692233bf5d415f5855948579158f077fb6140 Mon Sep 17 00:00:00 2001 From: Andrew Park Date: Mon, 18 May 2026 15:08:46 -0700 Subject: [PATCH] fix(swebench): namespace harness run_id by cell to stop concurrent collisions Concurrent hybrid SWE cells were colliding on the swebench-harness shared cache because run_id was keyed only on instance_id (`oj-`). The second cell to score the same task hit "1 instances already run, skipping..." and silently scored 0 with `reason: no_report` (or read the first cell's verdict on a write race). Previously papered over by manual cache wipes + rescores; this is the root-cause fix. run_id is now `oj--` when a cell name is supplied, falling back to the legacy `oj-` form for single-cell callers. Same-cell resumes still hit the harness cache (run_id is deterministic for a given cell + instance). Cell name is sanitized to filesystem-safe `[A-Za-z0-9._-]+` since it lands in both the `logs/run_evaluation//...` subtree and the `..json` summary filename. Plumbed cell_name through `runner._process -> score -> _score_swebench -> SWEBenchHarnessScorer -> _run_harness -> _build_run_id`. Regression test under `tests/agents/hybrid/test_swebench_run_id_isolation.py` pins: (a) different cells produce different run_ids, (b) same cell is stable, (c) legacy `None` cell preserves old format, (d) hostile cell names sanitize cleanly, (e) end-to-end wiring from scorer constructor through to the run_id. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/openjarvis/agents/hybrid/runner.py | 33 +++- .../evals/scorers/swebench_harness.py | 72 ++++++++- .../hybrid/test_swebench_run_id_isolation.py | 143 ++++++++++++++++++ 3 files changed, 236 insertions(+), 12 deletions(-) create mode 100644 tests/agents/hybrid/test_swebench_run_id_isolation.py diff --git a/src/openjarvis/agents/hybrid/runner.py b/src/openjarvis/agents/hybrid/runner.py index 736c2ae7..57384945 100644 --- a/src/openjarvis/agents/hybrid/runner.py +++ b/src/openjarvis/agents/hybrid/runner.py @@ -256,8 +256,19 @@ def _score_gaia(task: Dict[str, Any], answer: str) -> Dict[str, Any]: } -def _score_swebench(task: Dict[str, Any], answer: str) -> Dict[str, Any]: - """Modal-backed SWE-bench Verified harness scorer.""" +def _score_swebench( + task: Dict[str, Any], + answer: str, + cell_name: Optional[str] = None, +) -> Dict[str, Any]: + """Modal-backed SWE-bench Verified harness scorer. + + ``cell_name`` is passed through to :class:`SWEBenchHarnessScorer` so the + underlying ``run_id`` is unique per (cell, instance). Without it, + concurrent hybrid cells scoring the same task collide on the shared + swebench harness cache and the second cell silently scores 0 with + ``reason: no_report`` (or reads the first cell's verdict). + """ from openjarvis.evals.core.types import EvalRecord from openjarvis.evals.scorers.swebench_harness import ( SWEBenchHarnessScorer, @@ -275,7 +286,10 @@ def _score_swebench(task: Dict[str, Any], answer: str) -> Dict[str, Any]: category="agentic", metadata={"instance_id": task["task_id"]}, ) - scorer = SWEBenchHarnessScorer(timeout_s=int(os.environ.get("SWEBENCH_TIMEOUT_S", "1800"))) + scorer = SWEBenchHarnessScorer( + timeout_s=int(os.environ.get("SWEBENCH_TIMEOUT_S", "1800")), + cell_name=cell_name, + ) is_correct, details = scorer.score(record, answer) return { "success": bool(is_correct), @@ -284,11 +298,16 @@ def _score_swebench(task: Dict[str, Any], answer: str) -> Dict[str, Any]: } -def score(bench: str, task: Dict[str, Any], answer: str) -> Dict[str, Any]: +def score( + bench: str, + task: Dict[str, Any], + answer: str, + cell_name: Optional[str] = None, +) -> Dict[str, Any]: if bench == "gaia": return _score_gaia(task, answer) if bench in ("swebench-verified", "swebench_verified", "swebench"): - return _score_swebench(task, answer) + return _score_swebench(task, answer, cell_name=cell_name) raise ValueError(f"unknown bench: {bench!r}") @@ -573,7 +592,9 @@ def _run_cell_locked( scored: Optional[Dict[str, Any]] = None if do_score and row.get("error") is None: try: - scored = score(cell["bench"], task, row["answer"]) + scored = score( + cell["bench"], task, row["answer"], cell_name=cell_name, + ) except Exception as e: scored = { "success": False, "score": 0.0, diff --git a/src/openjarvis/evals/scorers/swebench_harness.py b/src/openjarvis/evals/scorers/swebench_harness.py index 1ef8f62b..d1188adb 100644 --- a/src/openjarvis/evals/scorers/swebench_harness.py +++ b/src/openjarvis/evals/scorers/swebench_harness.py @@ -271,8 +271,8 @@ def _find_report(cache: Path, instance_id: str, run_id: str) -> Optional[Dict[st """Find the harness's report JSON for one instance. swebench writes ``..json`` inside the - subprocess CWD. We use ``model_name_or_path="openjarvis-harness"``, - ``run_id=f"oj-{instance_id}"`` in :func:`_run_harness`. + subprocess CWD. We use ``model_name_or_path="openjarvis-harness"``; + ``run_id`` is built by :func:`_build_run_id`. """ fname = f"openjarvis-harness.{run_id}.json" p = cache / fname @@ -284,7 +284,53 @@ def _find_report(cache: Path, instance_id: str, run_id: str) -> Optional[Dict[st return None -def _run_harness(instance_id: str, patch: str, timeout_s: int) -> Dict[str, Any]: +_RUN_ID_SAFE_RE = re.compile(r"[^A-Za-z0-9._-]+") + + +def _sanitize_run_id_part(s: str) -> str: + """Reduce a free-form string to filesystem-safe ``[A-Za-z0-9._-]+``. + + Both the harness summary filename (``..json``) and the + per-instance log subtree (``logs/run_evaluation//...``) are + keyed on ``run_id``, so any character that breaks paths or globs will + silently corrupt the score. Strip leading/trailing dashes too — those + look fine but make filenames awkward to manage by hand. + """ + return _RUN_ID_SAFE_RE.sub("-", s).strip("-") + + +def _build_run_id(instance_id: str, cell_name: Optional[str]) -> str: + """Construct a swebench ``run_id`` unique per (cell, instance). + + The harness keys both its "already run, skipping" cache and its report + file path on ``run_id`` alone, so two concurrent cells scoring the + same ``instance_id`` with the same ``run_id`` collide: the second + cell's harness invocation finds the first's report on disk, skips + actual execution, and our caller silently reads the wrong verdict (or + ``no_report`` if the two cells race on the summary file write). See + :func:`_run_harness` for the full failure mode. + + With ``cell_name`` we emit ``oj--``, which keeps the + intra-cell resume cache working (same cell + same instance → same + run_id → harness cache hit) while making inter-cell collisions + impossible. Without ``cell_name`` we fall back to the legacy + ``oj-`` form for backwards compat with single-cell callers. + """ + safe_instance = _sanitize_run_id_part(instance_id) + if not cell_name: + return f"oj-{safe_instance}" + safe_cell = _sanitize_run_id_part(cell_name) + if not safe_cell: + return f"oj-{safe_instance}" + return f"oj-{safe_cell}-{safe_instance}" + + +def _run_harness( + instance_id: str, + patch: str, + timeout_s: int, + cell_name: Optional[str] = None, +) -> Dict[str, Any]: """Hand one prediction to ``python -m swebench.harness.run_evaluation``. Returns ``{"success": bool, "score": float, "details": dict}``. @@ -292,7 +338,7 @@ def _run_harness(instance_id: str, patch: str, timeout_s: int) -> Dict[str, Any] _apply_patches_once() backend = os.environ.get("SWEBENCH_BACKEND", "modal").lower() cache = _harness_cache_dir() - run_id = f"oj-{instance_id}" + run_id = _build_run_id(instance_id, cell_name) # Defend against stale reports: ``run_id`` is deterministic per # instance, the cache dir is shared across runs, and ``_find_report`` @@ -381,10 +427,17 @@ class SWEBenchHarnessScorer(Scorer): self, *, timeout_s: int = 1800, + cell_name: Optional[str] = None, judge_backend: object = None, # noqa: ARG002 — CLI factory compat judge_model: str = "", # noqa: ARG002 — CLI factory compat ) -> None: self._timeout_s = int(timeout_s) + # ``cell_name`` namespaces the ``run_id`` so concurrent cells scoring + # the same SWE instance don't collide on the harness's shared cache. + # See :func:`_build_run_id` for the failure mode this prevents. Pass + # the hybrid cell name (e.g. ``"skillorchestra-qwen36-opus47-swe-n100"``) + # or leave as ``None`` for single-cell callers. + self._cell_name = cell_name def score( self, @@ -406,10 +459,17 @@ class SWEBenchHarnessScorer(Scorer): if not instance_id: return False, {"reason": "missing_instance_id"} - result = _run_harness(instance_id, patch, self._timeout_s) + result = _run_harness( + instance_id, patch, self._timeout_s, cell_name=self._cell_name, + ) details = dict(result.get("details", {})) details["patch"] = patch return bool(result["success"]), details -__all__ = ["SWEBenchHarnessScorer", "extract_patch"] +__all__ = [ + "SWEBenchHarnessScorer", + "extract_patch", + "_build_run_id", + "_sanitize_run_id_part", +] diff --git a/tests/agents/hybrid/test_swebench_run_id_isolation.py b/tests/agents/hybrid/test_swebench_run_id_isolation.py new file mode 100644 index 00000000..7f5b916d --- /dev/null +++ b/tests/agents/hybrid/test_swebench_run_id_isolation.py @@ -0,0 +1,143 @@ +"""Regression test for SWE-bench ``run_id`` collisions across concurrent cells. + +Failure mode (observed 2026-05-18, twice): + Multiple hybrid SWE cells running concurrently against the Modal + ``swebench-harness`` shared its per-instance cache via a ``run_id`` + keyed only on ``instance_id``. Two cells scoring the same task hit + "1 instances already run, skipping..." on the second call; the + runner saw ``reason: no_report`` and scored that row 0 even when + the patch was correct. First seen with + ``minions-qwen27b-opus47-swe-n100`` vs the advisors cell; recurred + on 2026-05-18 across three of four ``qwen36`` SWE cells. + +Fix: ``_build_run_id(instance_id, cell_name)`` namespaces the run_id by +cell so concurrent cells can't collide while same-cell resumes still +hit the harness cache. + +This test pins the contract: + +1. Different cells + same instance → different run_ids (the bug). +2. Same cell + same instance → identical run_id (resume still works). +3. Both forms are filesystem-safe (no chars that would break the + ``logs/run_evaluation//...`` subtree or the + ``..json`` summary glob). +4. Cell name flows from :class:`SWEBenchHarnessScorer` into the run_id — + wiring regression guard, in case someone refactors and drops the + ``cell_name`` kwarg. +""" + +from __future__ import annotations + +import re + +import pytest + +from openjarvis.evals.scorers.swebench_harness import ( + SWEBenchHarnessScorer, + _build_run_id, + _sanitize_run_id_part, +) + + +# Filenames + harness log paths must match this. Slashes, spaces, colons, +# and other shell-hostile chars would silently corrupt scoring. +_SAFE_RE = re.compile(r"^[A-Za-z0-9._-]+$") + + +INSTANCE = "astropy__astropy-12907" +CELL_A = "advisors-qwen36-opus47-swe-n100" +CELL_B = "skillorchestra-qwen36-opus47-swe-n100" + + +def test_different_cells_produce_different_run_ids(): + """The core bug: two cells, same instance → must not collide.""" + a = _build_run_id(INSTANCE, CELL_A) + b = _build_run_id(INSTANCE, CELL_B) + assert a != b, ( + f"run_id collision between cells {CELL_A!r} and {CELL_B!r} " + f"on instance {INSTANCE!r}: both produced {a!r}" + ) + # Both must still contain the instance id so the harness logs are + # greppable by instance. + assert INSTANCE in a + assert INSTANCE in b + + +def test_same_cell_same_instance_is_stable(): + """Resume semantics: re-running the same cell must hit the cache.""" + assert _build_run_id(INSTANCE, CELL_A) == _build_run_id(INSTANCE, CELL_A) + + +def test_legacy_no_cell_name_preserves_old_format(): + """Single-cell callers (no ``cell_name``) keep the legacy ``oj-``.""" + assert _build_run_id(INSTANCE, None) == f"oj-{INSTANCE}" + assert _build_run_id(INSTANCE, "") == f"oj-{INSTANCE}" + + +@pytest.mark.parametrize( + "cell_name", + [ + CELL_A, + CELL_B, + "minions-qwen27b-opus47-swe-n100", + # Hostile inputs — must still produce something path-safe. + "weird/cell name with spaces", + "cell:with:colons", + "---leading-and-trailing---", + ], +) +def test_run_id_is_filesystem_safe(cell_name: str): + """Whatever we feed in, the run_id must be a clean path component.""" + rid = _build_run_id(INSTANCE, cell_name) + assert _SAFE_RE.match(rid), f"run_id {rid!r} contains unsafe chars" + + +def test_sanitizer_strips_unsafe_chars(): + assert _sanitize_run_id_part("a/b c:d") == "a-b-c-d" + assert _sanitize_run_id_part("---x---") == "x" + assert _sanitize_run_id_part("ok-cell.1_2") == "ok-cell.1_2" + + +def test_scorer_threads_cell_name_into_run_id(monkeypatch): + """End-to-end wiring: scorer constructor → ``_run_harness`` → run_id. + + Guards against a refactor that quietly drops the ``cell_name`` kwarg + on the scorer or stops forwarding it to ``_run_harness``. + """ + from openjarvis.evals.core.types import EvalRecord + from openjarvis.evals.scorers import swebench_harness as mod + + captured: dict = {} + + def fake_run_harness(instance_id, patch, timeout_s, cell_name=None): + captured["instance_id"] = instance_id + captured["cell_name"] = cell_name + captured["run_id"] = mod._build_run_id(instance_id, cell_name) + return {"success": True, "score": 1.0, "details": {}} + + monkeypatch.setattr(mod, "_run_harness", fake_run_harness) + + scorer = SWEBenchHarnessScorer(timeout_s=60, cell_name=CELL_A) + record = EvalRecord( + record_id=INSTANCE, + problem="", + reference="", + category="agentic", + metadata={"instance_id": INSTANCE}, + ) + # Minimal patch text that ``extract_patch`` will accept. + answer = "```diff\ndiff --git a/x b/x\n--- a/x\n+++ b/x\n@@ -0,0 +1 @@\n+x\n```" + scorer.score(record, answer) + + assert captured["cell_name"] == CELL_A + assert captured["instance_id"] == INSTANCE + assert CELL_A in captured["run_id"] + assert INSTANCE in captured["run_id"] + + # And the other cell name produces a *different* run_id end-to-end. + captured.clear() + scorer_b = SWEBenchHarnessScorer(timeout_s=60, cell_name=CELL_B) + scorer_b.score(record, answer) + assert captured["cell_name"] == CELL_B + assert CELL_B in captured["run_id"] + assert CELL_A not in captured["run_id"]