feat(python): implemented host tool event normalization and error checks
- Added `_normalize_host_tool_event` to `RpcClient` to remap transport tool events to executed host tools. - Updated worker tool-end handling to verify `is_error` is false before marking terminal actions complete. - Added test coverage in `test_client.py` and `test_worker.py` for host tool event normalization and errored review submission behavior.
This commit is contained in:
@@ -265,7 +265,13 @@ class GitHubClient:
|
||||
last_exc = exc
|
||||
log.warning(
|
||||
"transient github 5xx, retrying",
|
||||
extra={"method": method, "path": path, "attempt": attempt + 1, "delay": delay, "status": exc.status},
|
||||
extra={
|
||||
"method": method,
|
||||
"path": path,
|
||||
"attempt": attempt + 1,
|
||||
"delay": delay,
|
||||
"status": exc.status,
|
||||
},
|
||||
)
|
||||
time.sleep(delay)
|
||||
raise last_exc # type: ignore[misc]
|
||||
@@ -294,7 +300,13 @@ class GitHubClient:
|
||||
last_exc = exc
|
||||
log.warning(
|
||||
"transient github 5xx, retrying",
|
||||
extra={"method": method, "path": path, "attempt": attempt + 1, "delay": delay, "status": exc.status},
|
||||
extra={
|
||||
"method": method,
|
||||
"path": path,
|
||||
"attempt": attempt + 1,
|
||||
"delay": delay,
|
||||
"status": exc.status,
|
||||
},
|
||||
)
|
||||
await asyncio.sleep(delay)
|
||||
raise last_exc # type: ignore[misc]
|
||||
|
||||
@@ -500,14 +500,21 @@ def _run_rpc_blocking(
|
||||
|
||||
def _on_tool_end(event: ToolExecutionEndEvent) -> None:
|
||||
tool_name = event.tool_name
|
||||
if event.result is not None:
|
||||
# `tool_name` is transport-normalized by omp_rpc: an xd:// device
|
||||
# dispatch (`write xd://submit_pr_review`) reports the host tool that
|
||||
# ran, so terminal-action detection can match on host-tool names. A
|
||||
# failed execution (`is_error`) does not count as reaching the
|
||||
# terminal action — a rejected submit must still trigger the
|
||||
# completion reminder.
|
||||
ok = event.result is not None and not event.is_error
|
||||
if ok:
|
||||
tools_called.add(tool_name)
|
||||
log.info(
|
||||
"tool_end",
|
||||
extra={
|
||||
"issue": bindings.issue_key,
|
||||
"tool": tool_name,
|
||||
"ok": event.result is not None,
|
||||
"ok": ok,
|
||||
},
|
||||
)
|
||||
|
||||
|
||||
@@ -975,5 +975,3 @@ def test_route_non_directive_comment_carries_no_pragmas() -> None:
|
||||
)
|
||||
assert decision.directive is False
|
||||
assert decision.directive_pragmas == ()
|
||||
|
||||
|
||||
|
||||
@@ -650,7 +650,7 @@ async def test_run_rpc_stops_reminding_after_terminal_tool(tmp_path: Path, setti
|
||||
# driver registers the callback before prompt_and_wait, so we
|
||||
# replay it here.
|
||||
for cb in client._tool_end_callbacks:
|
||||
cb(SimpleNamespace(tool_name="gh_open_pr", result={}))
|
||||
cb(SimpleNamespace(tool_name="gh_open_pr", result={}, is_error=None))
|
||||
|
||||
# Capture the registered tool_end callback on the fake.
|
||||
original_on_tool_end = _FakeRpcClient.on_tool_execution_end
|
||||
@@ -759,7 +759,7 @@ async def test_run_rpc_review_pr_stops_after_submit_without_dirty_probe(
|
||||
|
||||
def _on_prompt(client: _FakeRpcClient, _prompt: str) -> None:
|
||||
for cb in client._tool_end_callbacks:
|
||||
cb(SimpleNamespace(tool_name="submit_pr_review", result={}))
|
||||
cb(SimpleNamespace(tool_name="submit_pr_review", result={}, is_error=None))
|
||||
|
||||
_FakeRpcClient.on_tool_execution_end = _record_tool_end # type: ignore[assignment]
|
||||
try:
|
||||
@@ -783,6 +783,55 @@ async def test_run_rpc_review_pr_stops_after_submit_without_dirty_probe(
|
||||
assert fake.prompts == ["kickoff"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_run_rpc_review_pr_still_reminds_when_submit_fails(tmp_path: Path, settings: Settings) -> None:
|
||||
"""An errored terminal-tool end event does not count as the terminal action.
|
||||
|
||||
omp_rpc normalizes xd:// device dispatches to the host-tool name, so a
|
||||
rejected `submit_pr_review` surfaces as an end event with `is_error=True`
|
||||
under its real name. Counting it would end the review task silently with
|
||||
no review submitted — the completion reminder must still fire.
|
||||
"""
|
||||
inputs, bindings = _make_inputs(tmp_path, settings, session_has_jsonl=False)
|
||||
original_on_tool_end = _FakeRpcClient.on_tool_execution_end
|
||||
|
||||
def _record_tool_end(self, cb) -> None:
|
||||
self._tool_end_callbacks = getattr(self, "_tool_end_callbacks", [])
|
||||
self._tool_end_callbacks.append(cb)
|
||||
|
||||
def _on_prompt(client: _FakeRpcClient, _prompt: str) -> None:
|
||||
for cb in client._tool_end_callbacks:
|
||||
cb(
|
||||
SimpleNamespace(
|
||||
tool_name="submit_pr_review",
|
||||
result={"content": [{"type": "text", "text": "GitHub rejected PR review: 422"}]},
|
||||
is_error=True,
|
||||
)
|
||||
)
|
||||
|
||||
_FakeRpcClient.on_tool_execution_end = _record_tool_end # type: ignore[assignment]
|
||||
try:
|
||||
_FakeRpcClient.on_prompt = staticmethod(_on_prompt) # type: ignore[attr-defined]
|
||||
loop = asyncio.new_event_loop()
|
||||
try:
|
||||
worker._run_rpc_blocking(
|
||||
inputs,
|
||||
task_kind="review_pr",
|
||||
prompt="kickoff",
|
||||
loop=loop,
|
||||
bindings=bindings, # type: ignore[arg-type]
|
||||
)
|
||||
finally:
|
||||
loop.close()
|
||||
finally:
|
||||
_FakeRpcClient.on_tool_execution_end = original_on_tool_end # type: ignore[assignment]
|
||||
delattr(_FakeRpcClient, "on_prompt")
|
||||
|
||||
fake = _FakeRpcClient.instances[0]
|
||||
assert len(fake.prompts) == 1 + settings.task_completion_max_reminders
|
||||
assert all("submit_pr_review" in p for p in fake.prompts[1:])
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Dirty-state watchdog
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user