From b4d5174385e43d502fe078cceb9eda09e82c7d5e Mon Sep 17 00:00:00 2001 From: David Metcalfe <80915+DavidMetcalfe@users.noreply.github.com> Date: Sat, 29 Aug 2026 12:49:28 -0700 Subject: [PATCH] fix(delegation): pin failure-status edge cases and document exit_reason enum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per-finding verdicts from the cross-vendor review of fix/status-fix (#97655/#97654): [1] Flash NIT (real, cheap) — FIXED. Added test_error_without_failed_flag_ marks_failed: an error string with the 'failed' key ABSENT (not False) must still be status=failed + exit_reason=error. The branch order (result.get('failed') or result.get('error')) already handles this; the test pins the error-alone path. [2] GPT-OSS SHOULD-FIX — PINNED. Added test_empty_error_with_summary_is_ completed: error='' is falsy so result.get('error') falls through to the summary-presence heuristic => status=completed. No code change; the existing branch is correct and the new test locks it in. [3] GPT-OSS SHOULD-FIX — VERIFIED, NO CHANGE. Grepped every delegation exit_reason consumer: * tools/delegation_live_log.py finalize() prints exit_reason generically and only special-cases == 'max_iterations' for a readable suffix. * tools/process_registry.py derives truncated as (truncated or exit_reason == 'max_iterations') — gated, not exhaustive. * tools/async_delegation.py passes exit_reason through generically. The gateway/status.py, cron/scheduler.py and run_agent.py 'exit_reason' hits are a DIFFERENT field (turn_exit_reason / gateway exit reason), not the delegation result's exit_reason. No exhaustive if/elif over the enum missing an 'error' case, so nothing to add. [4] GPT-OSS NIT — DONE. Enriched _run_single_child's docstring to enumerate status in {completed, interrupted, failed} and exit_reason in {completed, max_iterations, interrupted, error}, and added a compact enum comment at the result-entry construction. Verified the process_registry.py renderer comment (truncated <= exit_reason == 'max_iterations') still holds — the truncation flag is derived exactly that way, so no contradiction. [5] GPT-OSS NIT — REJECTED. The proposed 'fallback for legacy dicts that explicitly set failed=False' is not adopted. No consumer produces a result dict with an explicit failed=False and no summary while relying on completed semantics: run_agent.py sets failed=True only on genuine failure and omits the key on success (no failed=False producer). Also, the proposed elif would reintroduce ambiguity (explicit failed=False + no summary => 'completed'?) and diverge from the conservative else => 'failed'. result.get('failed') is falsy for both explicit-False and absent, so no distinction exists to preserve; the else is the correct default. Tests: 301 passed, 7 skipped (tests/tools -k 'delegate or process_registry'). TestDelegateFailedChildStatus: 6 passed. --- tests/tools/test_delegate.py | 41 ++++++++++++++++++++++++++++++++++++ tools/delegate_tool.py | 26 ++++++++++++++++++++++- 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/tests/tools/test_delegate.py b/tests/tools/test_delegate.py index a5aa206a65..ec09b6e374 100644 --- a/tests/tools/test_delegate.py +++ b/tests/tools/test_delegate.py @@ -770,6 +770,47 @@ class TestDelegateFailedChildStatus(unittest.TestCase): self.assertEqual(entry["exit_reason"], "error") self.assertFalse(entry["truncated"]) + def test_error_without_failed_flag_marks_failed(self): + """A child result that carries a non-empty error string but OMITS the + ``failed`` key entirely (not ``failed=False`` — the key is absent, as in + legacy/partial result dicts) must still be status=failed + exit_reason=error. + The status branch checks ``result.get('failed') or result.get('error')``, + so the error field alone has to win — otherwise a dropped ``failed`` key + would silently mislabel a provider rejection as budget exhaustion.""" + entry = self._delegate_single( + { + "final_response": "connection reset while streaming", + "completed": False, + "interrupted": False, + "error": "connection reset", + "api_calls": 2, + "messages": [], + } + ) + self.assertEqual(entry["status"], "failed") + self.assertEqual(entry["exit_reason"], "error") + self.assertFalse(entry["truncated"]) + + def test_empty_error_with_summary_is_completed(self): + """REGRESSION PIN: an empty-string ``error`` field must NOT be treated as + a failure. ``result.get('error')`` returns ``''`` which is falsy, so the + failure branch correctly falls through to the summary-presence heuristic. + Empty error + a real summary => status=completed, exit_reason=completed + (or max_iterations if completed=False), never 'error'.""" + entry = self._delegate_single( + { + "final_response": "work produced", + "completed": True, + "interrupted": False, + "error": "", + "api_calls": 2, + "messages": [], + } + ) + self.assertEqual(entry["status"], "completed") + self.assertEqual(entry["exit_reason"], "completed") + self.assertFalse(entry["truncated"]) + def test_genuine_truncation_stays_completed_max_iterations(self): """REGRESSION GUARD: a child that genuinely exhausts its iteration budget (completed=False, no failed flag, no error) but still returns a diff --git a/tools/delegate_tool.py b/tools/delegate_tool.py index 1d5f7f8ec2..89cab103c5 100644 --- a/tools/delegate_tool.py +++ b/tools/delegate_tool.py @@ -2579,7 +2579,27 @@ def _run_single_child( ) -> Dict[str, Any]: """ Run a pre-built child agent. Called from within a thread. - Returns a structured result dict. + Returns a structured result dict with a ``status`` and ``exit_reason`` + that are derived honestly from the child's structured completion fields. + + ``status`` ∈ {``"completed"``, ``"interrupted"``, ``"failed"``}: + * ``"completed"`` — the child reached a normal finish (may still have + hit its iteration budget; see ``exit_reason``). + * ``"interrupted"`` — the child was interrupted (``interrupted=True``). + * ``"failed"`` — a structured failure (``failed=True`` or a non-empty + ``error``) or a summary-less/invalid terminal state. + + ``exit_reason`` ∈ {``"completed"``, ``"max_iterations"``, ``"interrupted"``, + ``"error"``}: + * ``"completed"`` — normal finish. + * ``"max_iterations"`` — genuine per-child iteration-budget exhaustion + (``completed=False`` with no failure fields). + * ``"interrupted"`` — interrupted by the parent. + * ``"error"`` — provider rejection / terminal failure; NOT + budget exhaustion (this is the case #97655 fixed). + + ``truncated`` is derived as ``exit_reason == "max_iterations"`` only, so the + parent-visible truncation flag stays truthful for all of the above. """ child_start = time.monotonic() @@ -3238,6 +3258,10 @@ def _run_single_child( _output_tokens = getattr(child, "session_completion_tokens", 0) _model = getattr(child, "model", None) + # --- result entry contract (see _run_single_child docstring) --- + # status ∈ {completed, interrupted, failed} + # exit_reason ∈ {completed, max_iterations, interrupted, error} + # truncated is exactly (exit_reason == "max_iterations"). entry: Dict[str, Any] = { "task_index": task_index, "status": status,