fix(delegation): report failed children as failed, not completed
A subagent whose loop gave up on a structured failure (e.g. "API call failed after 3 retries: HTTP 524") returns that error message as final_response together with completed=False / failed=True / failure_reason. _run_single_child derived the batch-entry status from the summary alone (`elif summary and not _empty_sentinel: status = "completed"`), so the non-empty error text made the batch report show the task as "✓ status=completed" — the `failed` flag was never consulted anywhere in delegate_tool.py. Only the "(empty)" sentinel was mapped to failed. Fix, at the single status-determination choke point both the single-task and batch paths share: - `failed=True` on the child result now wins over a non-empty summary: status = "failed". - The child's classified failure_reason (rate_limit / billing / server_error / ...) is propagated onto the batch entry so the parent can tell a quota wall from a real task error without parsing prose. - exit_reason for a structured failure is "error" instead of falling through to "max_iterations" (which also wrongly set truncated=True). Successful children (completed=True, no failed flag) are untouched — covered by an explicit control test alongside the regression test, which is red on the old code and green with the fix.
This commit is contained in:
@@ -675,6 +675,75 @@ class TestDelegateObservability(unittest.TestCase):
|
||||
result = json.loads(delegate_task(goal="Test empty sentinel", parent_agent=parent))
|
||||
self.assertEqual(result["results"][0]["status"], "failed")
|
||||
|
||||
def test_failed_child_with_error_summary_marks_status_failed(self):
|
||||
"""Regression: a child whose loop gave up on a structured failure
|
||||
(``failed=True``, ``completed=False``, e.g. "API call failed after 3
|
||||
retries: HTTP 524") returns that error message as final_response.
|
||||
Status was derived from summary alone, so the non-empty error text
|
||||
made the batch report show the task as ✓ status=completed. The
|
||||
``failed`` flag must win over a non-empty summary."""
|
||||
parent = _make_mock_parent(depth=0)
|
||||
|
||||
with patch("run_agent.AIAgent") as MockAgent:
|
||||
mock_child = MagicMock()
|
||||
mock_child.model = "claude-sonnet-4-6"
|
||||
mock_child.session_prompt_tokens = 0
|
||||
mock_child.session_completion_tokens = 0
|
||||
mock_child.run_conversation.return_value = {
|
||||
"final_response": (
|
||||
"API call failed after 3 retries: HTTP 524 — origin timeout"
|
||||
),
|
||||
"completed": False,
|
||||
"failed": True,
|
||||
"error": "HTTP 524 — origin timeout",
|
||||
"failure_reason": "server_error",
|
||||
"interrupted": False,
|
||||
"api_calls": 3,
|
||||
"messages": [],
|
||||
}
|
||||
MockAgent.return_value = mock_child
|
||||
|
||||
result = json.loads(
|
||||
delegate_task(goal="Test failed child", parent_agent=parent)
|
||||
)
|
||||
entry = result["results"][0]
|
||||
self.assertEqual(entry["status"], "failed")
|
||||
# The classified reason must survive into the batch entry so the
|
||||
# parent can tell a quota wall from a real task error.
|
||||
self.assertEqual(entry["failure_reason"], "server_error")
|
||||
self.assertEqual(entry["error"], "HTTP 524 — origin timeout")
|
||||
# A structured failure is not budget truncation.
|
||||
self.assertEqual(entry["exit_reason"], "error")
|
||||
self.assertFalse(entry["truncated"])
|
||||
|
||||
def test_successful_child_still_completed(self):
|
||||
"""Control for the failed-flag check: a child that succeeds
|
||||
(``completed=True``, no ``failed`` flag) must keep reporting
|
||||
status=completed — the fix must not change success behavior."""
|
||||
parent = _make_mock_parent(depth=0)
|
||||
|
||||
with patch("run_agent.AIAgent") as MockAgent:
|
||||
mock_child = MagicMock()
|
||||
mock_child.model = "claude-sonnet-4-6"
|
||||
mock_child.session_prompt_tokens = 0
|
||||
mock_child.session_completion_tokens = 0
|
||||
mock_child.run_conversation.return_value = {
|
||||
"final_response": "All done.",
|
||||
"completed": True,
|
||||
"interrupted": False,
|
||||
"api_calls": 2,
|
||||
"messages": [],
|
||||
}
|
||||
MockAgent.return_value = mock_child
|
||||
|
||||
result = json.loads(
|
||||
delegate_task(goal="Test success control", parent_agent=parent)
|
||||
)
|
||||
entry = result["results"][0]
|
||||
self.assertEqual(entry["status"], "completed")
|
||||
self.assertEqual(entry["exit_reason"], "completed")
|
||||
self.assertNotIn("failure_reason", entry)
|
||||
|
||||
|
||||
class TestDelegateFailedChildStatus(unittest.TestCase):
|
||||
"""Honest status / exit_reason for failed subagents (issue #97655).
|
||||
|
||||
@@ -3317,6 +3317,12 @@ def _run_single_child(
|
||||
)
|
||||
if status == "failed":
|
||||
entry["error"] = result.get("error", "Subagent did not produce a response.")
|
||||
# Classified reason from the child loop (e.g. "rate_limit",
|
||||
# "billing", "server_error") — lets the parent distinguish a
|
||||
# quota wall from a real task error without parsing prose.
|
||||
_failure_reason = result.get("failure_reason")
|
||||
if isinstance(_failure_reason, str) and _failure_reason:
|
||||
entry["failure_reason"] = _failure_reason
|
||||
|
||||
# T1-24: schema-validation outcome — emitted ONLY when a schema was
|
||||
# requested, so legacy (schema-less) payloads keep their exact shape.
|
||||
|
||||
Reference in New Issue
Block a user