diff --git a/roboco/runtime/orchestrator.py b/roboco/runtime/orchestrator.py index 38626e69..a970b5f2 100644 --- a/roboco/runtime/orchestrator.py +++ b/roboco/runtime/orchestrator.py @@ -4626,11 +4626,20 @@ class AgentOrchestrator: """ from roboco.db.base import get_session_factory from roboco.services.task import TaskService - from roboco.utils.converters import require_uuid + from roboco.utils.converters import InvalidIdentifierError, require_uuid try: task_id = require_uuid(task_id_str) - except Exception: + except InvalidIdentifierError as exc: + # A malformed task_id_str is a bad identifier, not a transient + # failure — log it so the drop is visible instead of swallowed, + # then no-op (nothing to release). Other exceptions still fall + # through to the broad catch below (#25). + logger.warning( + "stopped agent claim had malformed task id", + task_id_str=task_id_str, + error=str(exc), + ) return try: factory = get_session_factory() diff --git a/roboco/utils/converters.py b/roboco/utils/converters.py index 72d7a0a2..81adc629 100644 --- a/roboco/utils/converters.py +++ b/roboco/utils/converters.py @@ -8,6 +8,16 @@ from typing import Any from uuid import UUID as PythonUUID +class InvalidIdentifierError(ValueError): + """A malformed/None identifier reached UUID coercion. + + A ``ValueError`` subclass so existing ``except ValueError`` callers keep + working, but typed so a caller can distinguish a bad identifier from any + other exception instead of broad-catching and silently swallowing it + (#25). + """ + + def require_uuid(value: Any) -> PythonUUID: """ Convert SQLAlchemy UUID to Python UUID, raising if None. @@ -19,13 +29,19 @@ def require_uuid(value: Any) -> PythonUUID: Python UUID Raises: - ValueError: If value is None or cannot be converted + InvalidIdentifierError: If value is None or cannot be parsed as a UUID. + A ``ValueError`` subclass, so existing ``except ValueError`` / + ``except Exception`` callers are unaffected, but typed so callers + can handle a bad identifier distinctly instead of swallowing it. """ if value is None: - raise ValueError("UUID value cannot be None") + raise InvalidIdentifierError("UUID value cannot be None") if isinstance(value, PythonUUID): return value - return PythonUUID(str(value)) + try: + return PythonUUID(str(value)) + except (ValueError, AttributeError, TypeError) as exc: + raise InvalidIdentifierError(f"invalid UUID identifier: {value!r}") from exc def repo_key(git_url: str) -> str: diff --git a/tests/unit/utils/test_converters.py b/tests/unit/utils/test_converters.py index fad25f8f..8b98f7c5 100644 --- a/tests/unit/utils/test_converters.py +++ b/tests/unit/utils/test_converters.py @@ -6,6 +6,7 @@ from uuid import uuid4 import pytest from roboco.utils.converters import ( + InvalidIdentifierError, require_uuid, to_python_uuid, to_python_uuid_list, @@ -27,6 +28,27 @@ def test_require_uuid_raises_for_none() -> None: require_uuid(None) +def test_require_uuid_none_raises_typed_identifier_error() -> None: + """#25: a None identifier raises the typed InvalidIdentifierError (a + ValueError subclass), not a bare ValueError, so callers can distinguish a + bad identifier from any other exception instead of broad-catching.""" + with pytest.raises(InvalidIdentifierError, match="cannot be None"): + require_uuid(None) + + +def test_require_uuid_unparseable_raises_typed_identifier_error() -> None: + """#25: an unparseable identifier raises InvalidIdentifierError, not a bare + ValueError — the dropped-identifier pattern must surface as a typed error.""" + with pytest.raises(InvalidIdentifierError, match="invalid UUID identifier"): + require_uuid("not-a-uuid") + + +def test_invalid_identifier_error_is_value_error() -> None: + """Back-compat: InvalidIdentifierError subclasses ValueError so existing + `except ValueError` callers keep catching it.""" + assert issubclass(InvalidIdentifierError, ValueError) + + def test_to_python_uuid_returns_none_for_none() -> None: assert to_python_uuid(None) is None