fix(skillopt-sleep): also surface codex failures on the tool-call rollout path
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 <noreply@anthropic.com>
This commit is contained in:
@@ -890,16 +890,27 @@ class CodexCliBackend(CliBackend):
|
|||||||
if self.model:
|
if self.model:
|
||||||
cmd += ["-m", self.model]
|
cmd += ["-m", self.model]
|
||||||
cmd += ["--", prompt]
|
cmd += ["--", prompt]
|
||||||
|
self.last_call_error = ""
|
||||||
|
proc = None
|
||||||
try:
|
try:
|
||||||
subprocess.run(cmd, capture_output=True, text=True, timeout=self.timeout, cwd=work)
|
proc = subprocess.run(cmd, capture_output=True, text=True, timeout=self.timeout, cwd=work)
|
||||||
except Exception:
|
except subprocess.TimeoutExpired:
|
||||||
pass
|
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 = ""
|
resp = ""
|
||||||
try:
|
try:
|
||||||
with open(out_path, encoding="utf-8") as f:
|
with open(out_path, encoding="utf-8") as f:
|
||||||
resp = f.read().strip()
|
resp = f.read().strip()
|
||||||
except Exception:
|
except Exception:
|
||||||
resp = ""
|
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
|
self._tokens += len(prompt) // 4 + len(resp) // 4
|
||||||
called: List[str] = []
|
called: List[str] = []
|
||||||
if os.path.exists(calllog):
|
if os.path.exists(calllog):
|
||||||
|
|||||||
@@ -744,6 +744,29 @@ class TestCodexBackend(unittest.TestCase):
|
|||||||
self.assertIn("refresh_token_reused", be.last_call_error) # surfaced for the operator
|
self.assertIn("refresh_token_reused", be.last_call_error) # surfaced for the operator
|
||||||
self.assertEqual(calls["n"], 1) # failed fast, no wasted retries
|
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):
|
class TestMultiRolloutAndBudget(unittest.TestCase):
|
||||||
def test_rolloutset_stats(self):
|
def test_rolloutset_stats(self):
|
||||||
|
|||||||
Reference in New Issue
Block a user