fix(delegation): pin failure-status edge cases and document exit_reason enum
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.
This commit is contained in:
@@ -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
|
||||
|
||||
+25
-1
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user