Merge pull request #14 from dreamrec/claude/v2.0-pr25-drop-error-key-fallback

remove: legacy "error"-key fallback in is_tool_error_result (F-12, BREAKING)

Author: dreamrec

td_component/tdpilot_api_agent.py

@@ -29,32 +29,20 @@
 from collections.abc import Callable
 from typing import Any
 
-# Phase 3 (F-12) — soft-import the tool-error sentinel helper. The
-# dispatcher module owns the canonical predicate; the agent loop just
-# calls it. Soft-import so a stripped-down test embed without the
-# dispatcher module still loads. The fallback mirrors the dispatcher's
-# v1.10.0 DeprecationWarning behavior so embeds that exercise this
-# shim see the same warning surface as production.
+# F-12 — soft-import the tool-error sentinel helper. The dispatcher
+# module owns the canonical predicate; the agent loop just calls it.
+# Soft-import so a stripped-down test embed without the dispatcher
+# module still loads. The fallback mirrors the dispatcher's v2.0
+# semantics: only the explicit ``_tool_error`` sentinel marks failure.
 try:
     from tdpilot_api_dispatcher import is_tool_error_result  # type: ignore[import-not-found]
 except ImportError:
-    import warnings as _warnings_shim
 
     def is_tool_error_result(result):  # type: ignore[misc]
         if not isinstance(result, dict):
             return False
         if "_tool_error" in result:
             return bool(result["_tool_error"])
-        if "error" in result:
-            _warnings_shim.warn(
-                "Tool result was classified as an error via the legacy "
-                "'error' key. Update your handler to emit "
-                "{'_tool_error': True, 'error': '...'} explicitly. "
-                "The legacy fallback is removed in TDPilot DPSK4 v2.0.",
-                DeprecationWarning,
-                stacklevel=2,
-            )
-            return True
         return False
 
 # ---------------------------------------------------------------------------

td_component/tdpilot_api_batch.py

@@ -28,31 +28,19 @@
 import time
 from typing import Any
 
-# Phase 3 (F-12) — soft-import the tool-error sentinel helper. Same
-# story as agent.py: the dispatcher module owns the canonical
-# predicate. The fallback mirrors the dispatcher's v1.10.0
-# DeprecationWarning behavior so embeds that exercise this shim see
-# the same warning surface as production.
+# F-12 — soft-import the tool-error sentinel helper. Same story as
+# agent.py: the dispatcher module owns the canonical predicate. The
+# fallback mirrors the dispatcher's v2.0 semantics: only the explicit
+# ``_tool_error`` sentinel marks failure.
 try:
     from tdpilot_api_dispatcher import is_tool_error_result  # type: ignore[import-not-found]
 except ImportError:
-    import warnings as _warnings_shim
 
     def is_tool_error_result(result):  # type: ignore[misc]
         if not isinstance(result, dict):
             return False
         if "_tool_error" in result:
             return bool(result["_tool_error"])
-        if "error" in result:
-            _warnings_shim.warn(
-                "Tool result was classified as an error via the legacy "
-                "'error' key. Update your handler to emit "
-                "{'_tool_error': True, 'error': '...'} explicitly. "
-                "The legacy fallback is removed in TDPilot DPSK4 v2.0.",
-                DeprecationWarning,
-                stacklevel=2,
-            )
-            return True
         return False
 
 

td_component/tdpilot_api_dispatcher.py

@@ -25,7 +25,6 @@
 from __future__ import annotations
 
 import traceback
-import warnings
 from collections.abc import Callable
 from typing import Any
 
@@ -52,47 +51,30 @@ def _scrub(s: str) -> str:
     return redact_paths(redact(s))
 
 
-# Phase 3 (F-12) — explicit tool-error sentinel. Pre-1.8.1 the agent
-# loop and tool_batch checked ``"error" in result`` to decide whether
-# a tool call failed, which misclassifies any handler whose successful
-# result legitimately contains an "error" field (e.g. ``td_get_errors``
-# returning a list of TD compile errors). The new convention sets
-# ``_tool_error: True`` on results that represent a dispatch / handler
-# failure; the agent loop checks the sentinel first and falls back to
-# the legacy ``"error"`` key.
-#
-# v1.10.0: the legacy fallback now emits ``DeprecationWarning`` to
-# nudge external dispatcher integrations / user-authored handlers off
-# the brittle heuristic. The fallback drops entirely in v2.0.
+# F-12 — explicit tool-error sentinel. Handlers signal a dispatch /
+# handler failure by stamping ``_tool_error: True`` on the result;
+# this is the authoritative signal the agent loop checks. A handler
+# whose successful result legitimately contains an ``error`` field
+# (e.g. ``td_get_errors`` returning a list of TD compile errors) is
+# classified as success — the sentinel is the only thing that marks
+# failure. v1.10.0 emitted a ``DeprecationWarning`` for the legacy
+# ``"error" in result`` heuristic; v2.0 removed the fallback entirely.
 TOOL_ERROR_KEY = "_tool_error"
 
 
 def is_tool_error_result(result: Any) -> bool:
     """True when ``result`` represents a tool-call failure that the
     agent loop should signal back to the model with ``is_error=True``.
 
-    Resolution order:
-      1. Explicit ``_tool_error`` sentinel — authoritative when present
-         (allowing handlers to flag an error WITHOUT carrying an
-         ``error`` key, or to flag success even WITH an ``error`` key).
-      2. Legacy ``error`` key — backward-compat shim for handlers that
-         haven't been updated. **Deprecated in v1.10.0**, removed in v2.0.
-         Reaching this branch emits a ``DeprecationWarning``.
+    The ``_tool_error`` sentinel is authoritative — a handler must
+    stamp it explicitly to mark failure. A bare ``"error"`` key alone
+    is no longer a failure signal (the legacy v1.x fallback was
+    removed in v2.0).
     """
     if not isinstance(result, dict):
         return False
     if TOOL_ERROR_KEY in result:
         return bool(result[TOOL_ERROR_KEY])
-    if "error" in result:
-        warnings.warn(
-            "Tool result was classified as an error via the legacy "
-            "'error' key. Update your handler to emit "
-            "{'_tool_error': True, 'error': '...'} explicitly. "
-            "The legacy fallback is removed in TDPilot DPSK4 v2.0.",
-            DeprecationWarning,
-            stacklevel=2,
-        )
-        return True
     return False
 
 

tests/test_tool_error_sentinel.py

@@ -7,21 +7,15 @@
 of compile errors).
 
 PR-17 introduced ``_tool_error: bool`` as the authoritative flag.
-``is_tool_error_result(result)`` checks the sentinel first and falls
-back to the legacy ``error`` key.
-
-**v1.10.0 (PR-24)**: the legacy fallback now emits
-``DeprecationWarning`` to nudge external dispatcher integrations off
-the brittle heuristic. The fallback is removed entirely in v2.0.
+``is_tool_error_result(result)`` checks the sentinel; v1.10.0 emitted
+a ``DeprecationWarning`` for the legacy ``"error" in result`` fallback;
+**v2.0 (PR-25) removed the fallback entirely** — only the explicit
+sentinel marks failure.
 
 Tests cover:
   * ``is_tool_error_result`` truth table (sentinel-True, sentinel-False,
-    no-error-key, non-dict input). The legacy fallback paths are
-    tested separately so the ``DeprecationWarning`` is asserted at
-    its emission site.
-  * Legacy ``"error"`` key still classifies as an error AND emits
-    ``DeprecationWarning`` (v1.10.0).
-  * Sentinel-driven results never emit ``DeprecationWarning``.
+    no-error-key, non-dict input, AND a bare ``"error"`` key — which
+    no longer marks failure post-v2.0).
   * Dispatcher synthetic errors carry the sentinel.
   * The agent loop (``tdpilot_api_agent``) imports + uses the helper.
   * ``tool_batch`` imports + uses the helper.
@@ -64,6 +58,14 @@
         ({"_tool_error": 0}, False),
         ({"_tool_error": ""}, False),
         ({"_tool_error": None}, False),
+        # v2.0 (PR-25): a bare ``error`` key no longer marks failure —
+        # the sentinel is the only signal. Handlers that emit
+        # ``{"error": "..."}`` without the sentinel are silently
+        # treated as success. Pre-v2.0 these classified as True via
+        # the legacy heuristic; v1.10.0 emitted DeprecationWarning;
+        # v2.0 removed the fallback entirely.
+        ({"error": "Unknown tool"}, False),
+        ({"error": ""}, False),
         # No sentinel, no error key.
         ({"ok": True, "path": "/project1"}, False),
         ({}, False),
@@ -75,9 +77,12 @@
     ],
 )
 def test_is_tool_error_result_truth_table(result, expected):
-    """Sentinel-driven cases must never emit a warning."""
+    """No path through ``is_tool_error_result`` should emit a warning
+    in v2.0 — the legacy ``DeprecationWarning`` was removed alongside
+    the fallback. ``simplefilter('error')`` is kept as a regression
+    guard against accidentally re-introducing one."""
     with warnings.catch_warnings():
-        warnings.simplefilter("error")  # any DeprecationWarning fails the test
+        warnings.simplefilter("error")
         assert disp.is_tool_error_result(result) is expected
 
 
@@ -87,47 +92,6 @@ def test_tool_error_key_constant_is_dunder_underscore():
     assert disp.TOOL_ERROR_KEY == "_tool_error"
 
 
-# ---------------------------------------------------------------------------
-# v1.10.0 (PR-24) — legacy "error"-key fallback emits DeprecationWarning.
-# These cases used to live in the truth-table parametrize above; they were
-# extracted here so the warning surface is asserted at its emission site.
-# In v2.0 these tests flip to expecting `False` and the warning assertion
-# goes away (the fallback is removed entirely).
-# ---------------------------------------------------------------------------
-
-
-@pytest.mark.parametrize(
-    "result",
-    [
-        {"error": "Unknown tool"},
-        {"error": ""},  # empty string still triggers (key-presence semantics)
-    ],
-)
-def test_legacy_error_key_classifies_as_error_with_deprecation_warning(result):
-    with pytest.warns(DeprecationWarning, match="legacy 'error' key"):
-        assert disp.is_tool_error_result(result) is True
-
-
-def test_sentinel_path_emits_no_deprecation_warning():
-    """Sentinel-driven classification (the new convention) must stay
-    silent — only the legacy fallback is deprecated."""
-    with warnings.catch_warnings():
-        warnings.simplefilter("error")  # any DeprecationWarning fails the test
-        assert disp.is_tool_error_result({"_tool_error": True}) is True
-        assert disp.is_tool_error_result({"_tool_error": False}) is False
-        assert disp.is_tool_error_result({"_tool_error": True, "error": "x"}) is True
-        assert disp.is_tool_error_result({"_tool_error": False, "error": "x"}) is False
-
-
-def test_no_warning_on_no_error_key():
-    """A dict with neither sentinel nor error key returns False
-    silently — there's nothing to deprecate."""
-    with warnings.catch_warnings():
-        warnings.simplefilter("error")
-        assert disp.is_tool_error_result({"ok": True}) is False
-        assert disp.is_tool_error_result({}) is False
-
-
 # ---------------------------------------------------------------------------
 # Dispatcher synthetic errors carry the sentinel
 # ---------------------------------------------------------------------------