From 9fa0716c72b7c67fe6099e75f46a354e2319245c Mon Sep 17 00:00:00 2001 From: Daniel Martinez Date: Sat, 27 Jun 2026 23:56:11 -0500 Subject: [PATCH] fix(skillopt-sleep): also surface codex failures on the tool-call rollout path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up from a fresh-context review of the prior commit: CodexCliBackend.attempt_with_tools (the rollout path for tool-requiring tasks) ran codex exec inline, swallowed all exceptions, and never set last_call_error — so an auth/model/version failure on the tool path still produced a silent empty->0 with no diagnostic signal, the exact failure class the prior commit fixed for the _call path. Now it surfaces timeout/exception/non-zero-exit via last_call_error (response stays empty; never leaks the CLI error text), so a failed tool rollout shows up in diagnostics.json. Adds a regression test. Co-Authored-By: Claude Opus 4.8 --- skillopt_sleep/backend.py | 17 ++++++++++++++--- tests/test_sleep_engine.py | 23 +++++++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/skillopt_sleep/backend.py b/skillopt_sleep/backend.py index d5658c6..b1c7208 100644 --- a/skillopt_sleep/backend.py +++ b/skillopt_sleep/backend.py @@ -890,16 +890,27 @@ class CodexCliBackend(CliBackend): if self.model: cmd += ["-m", self.model] cmd += ["--", prompt] + self.last_call_error = "" + proc = None try: - subprocess.run(cmd, capture_output=True, text=True, timeout=self.timeout, cwd=work) - except Exception: - pass + proc = subprocess.run(cmd, capture_output=True, text=True, timeout=self.timeout, cwd=work) + except subprocess.TimeoutExpired: + self.last_call_error = f"codex exec (tools) timed out after {self.timeout}s" + except Exception as exc: # noqa: BLE001 + self.last_call_error = f"codex exec (tools) failed: {exc}" resp = "" try: with open(out_path, encoding="utf-8") as f: resp = f.read().strip() except Exception: resp = "" + # Surface a failed tool-rollout the SAME way _call does: an auth/model/version + # failure on this path must show up in diagnostics (call_error), not vanish as a + # silent empty->0 scored as a failed rollout. Response stays "" (never the error text). + if not resp and not self.last_call_error and proc is not None and proc.returncode != 0: + self.last_call_error = ( + f"codex exec (tools) exited {proc.returncode}: {(proc.stderr or '')[:500]}" + ) self._tokens += len(prompt) // 4 + len(resp) // 4 called: List[str] = [] if os.path.exists(calllog): diff --git a/tests/test_sleep_engine.py b/tests/test_sleep_engine.py index 113bc8e..bd5b971 100644 --- a/tests/test_sleep_engine.py +++ b/tests/test_sleep_engine.py @@ -744,6 +744,29 @@ class TestCodexBackend(unittest.TestCase): self.assertIn("refresh_token_reused", be.last_call_error) # surfaced for the operator self.assertEqual(calls["n"], 1) # failed fast, no wasted retries + def test_codex_attempt_with_tools_surfaces_error_not_silent(self): + """A failed tool-rollout (non-zero codex exec) on the tool path must set + last_call_error and return an empty response — not a silent empty->0 the + diagnostics can't see (the gap a _call-only fix would otherwise leave).""" + from skillopt_sleep.backend import CodexCliBackend + + def fake_run(cmd, **kwargs): + class Proc: + returncode = 1 + stdout = "" + stderr = "ERROR codex_core::auth: 401 Unauthorized: refresh_token_reused" + return Proc() # writes nothing to out_path -> empty response + + be = CodexCliBackend(codex_path="codex") + task = TaskRecord(id="t", project="/p", intent="answer the question", + reference_kind="rule", + judge={"checks": [{"op": "tool_called", "arg": "search"}]}) + with mock.patch("skillopt_sleep.backend.subprocess.run", side_effect=fake_run): + resp, called = be.attempt_with_tools(task, "", "", ["search"]) + self.assertEqual(resp, "") # no leaked error text as a "response" + self.assertIn("exited 1", be.last_call_error) # failure surfaced for diagnostics + self.assertEqual(called, []) # no tool actually ran + class TestMultiRolloutAndBudget(unittest.TestCase): def test_rolloutset_stats(self):