From 1b6ea1a2c2b4e48a15ea392e1f3f94331fba1c57 Mon Sep 17 00:00:00 2001 From: itskaism Date: Sun, 16 Aug 2026 16:49:42 +0900 Subject: [PATCH] fix(delegation): report failed children as failed, not completed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- tests/tools/test_delegate.py | 69 ++++++++++++++++++++++++++++++++++++ tools/delegate_tool.py | 6 ++++ 2 files changed, 75 insertions(+) diff --git a/tests/tools/test_delegate.py b/tests/tools/test_delegate.py index 598819cf6e..63d28cc121 100644 --- a/tests/tools/test_delegate.py +++ b/tests/tools/test_delegate.py @@ -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). diff --git a/tools/delegate_tool.py b/tools/delegate_tool.py index 89cab103c5..4177f74d22 100644 --- a/tools/delegate_tool.py +++ b/tools/delegate_tool.py @@ -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.