From 01df9836ee6d094ac469a6f137a2db350db93f6d Mon Sep 17 00:00:00 2001 From: quinn-galileo Date: Mon, 19 Oct 2026 12:34:57 -0400 Subject: [PATCH 1/9] feat: clear errors for evaluator permission denials and add scope changes Add EvaluatorPermissionError (a ForbiddenError subclass) and translate 403s from evaluator create, update, delete and scope changes into it, with a hint that names the role needed. Reword client-side not-found errors to cover evaluators that are not shared with the caller's projects, handle a 422 from the scorer post on the LLM and code create paths, and add Evaluator.set_scope over PUT /scorers/{scorer_id}/scope. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 11 + README.md | 23 ++ src/splunk_ao/__init__.py | 2 + src/splunk_ao/evaluator.py | 121 +++++++- src/splunk_ao/evaluators.py | 85 +++++- src/splunk_ao/exceptions.py | 12 + tests/test_evaluator.py | 2 +- tests/test_evaluator_permissions.py | 453 ++++++++++++++++++++++++++++ tests/test_evaluators.py | 2 +- 9 files changed, 696 insertions(+), 15 deletions(-) create mode 100644 tests/test_evaluator_permissions.py diff --git a/CHANGELOG.md b/CHANGELOG.md index d3702c9e..cad0609f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `project_ids` and no current project raises `ValidationError` before any request is sent. - `Evaluator.is_global` and `Evaluator.scope_projects` report the scope the API returned, after `create()`, `get()`, `list()` and `refresh()`. +- `EvaluatorPermissionError`, a `ForbiddenError` subclass, replaces the generic 403 message when creating, + updating, deleting or re-scoping an evaluator is refused. It says what is needed: org admin to create a global + evaluator (or pass `project_ids`), the owner or editor role on the projects for scoped create, update, delete and + scope changes. The server response body is kept in the message. +- `Evaluator.set_scope(project_ids=[...])` and `set_scope(is_global=True)` replace an evaluator's scope through + `PUT /scorers/{scorer_id}/scope`. Promoting to global is org admin only. ### Changed @@ -38,6 +44,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `Evaluators.delete_evaluator()` and `Evaluator.refresh()` now say "not found, or not shared with your projects" + when the evaluator is missing, since the server hides evaluators outside the caller's projects. `Evaluator.get()` + still returns `None` in that case. A 404 from update, delete or `set_scope` stays a `NotFoundError`. +- Creating an LLM or code evaluator no longer fails with an `AttributeError` when the API answers with a validation + error; the detail is raised as a `ValueError`, as it already was for decision evaluators. - `SplunkAOTracingProcessor` (OpenAI Agents) now records Responses API LLM spans, the Agents SDK default, as chat messages: tool calls and tool results keep their own roles instead of one JSON `user` message, the output is a single assistant message, and workflow/agent spans show the diff --git a/README.md b/README.md index 0199c87c..4c09a213 100644 --- a/README.md +++ b/README.md @@ -627,6 +627,29 @@ run_experiment( ) ``` +#### Evaluators + +New evaluators are scoped to the current project (`SPLUNK_AO_PROJECT_ID` or `SPLUNK_AO_PROJECT`) by default. Pass +`project_ids` to choose other projects, or `is_global=True` for a global evaluator. Only org admins can create global +evaluators, and changing or deleting an evaluator needs the owner or editor role on one of its projects. + +```python +from splunk_ao import EvaluatorPermissionError +from splunk_ao.__future__ import LlmEvaluator + +try: + evaluator = LlmEvaluator( + name="politeness", prompt="Is the answer polite?", project_ids=[""] + ).create() + # Replace the scope: scoped to exactly these projects (is_global=True is org admins only). + evaluator.set_scope(project_ids=["", ""]) +except EvaluatorPermissionError as error: + print(error) # says which role is missing and how to fix it +``` + +`EvaluatorPermissionError` subclasses `ForbiddenError`. Looking up an evaluator by name returns `None` when it does +not exist or is not shared with your projects. + ### Sessions Sessions group related traces under the same conversation. To explicitly create diff --git a/src/splunk_ao/__init__.py b/src/splunk_ao/__init__.py index 3acf0294..354e0402 100644 --- a/src/splunk_ao/__init__.py +++ b/src/splunk_ao/__init__.py @@ -57,6 +57,7 @@ AuthenticationError, BadRequestError, ConflictError, + EvaluatorPermissionError, ForbiddenError, NotFoundError, RateLimitError, @@ -129,6 +130,7 @@ "DecisionVersionOutcomeUnknownError", "Document", "Evaluator", + "EvaluatorPermissionError", "Experiment", "ForbiddenError", "Integration", diff --git a/src/splunk_ao/evaluator.py b/src/splunk_ao/evaluator.py index 181b1fe9..e2312186 100644 --- a/src/splunk_ao/evaluator.py +++ b/src/splunk_ao/evaluator.py @@ -20,10 +20,14 @@ from splunk_ao.config import SplunkAOConfig from splunk_ao.configuration import Configuration from splunk_ao.evaluators import ( + _SCOPE_CHANGE_DENIED_HINT, DecisionAnswerOptions, DecisionVersionOutcomeUnknownError, Evaluators, + _create_denied_hint, + _edit_denied_hint, _resolve_create_scope, + _translate_forbidden, normalize_decision_answer_options, resolve_decision_output_type, validate_decision_request, @@ -32,6 +36,7 @@ create_code_scorer_version_scorers_scorer_id_version_code_post, create_scorers_post, get_validate_code_scorer_task_result_scorers_code_validate_task_id_get, + set_scorer_scope_scorers_scorer_id_scope_put, update_scorers_scorer_id_patch, validate_code_scorer_scorers_code_validate_post, ) @@ -49,6 +54,7 @@ ScorerTypes, TaskResultStatus, UpdateScorerRequest, + UpdateScorerScopeRequest, ) from splunk_ao.resources.models.invalid_result import InvalidResult from splunk_ao.resources.types import UNSET, File, Unset @@ -285,7 +291,7 @@ def get(cls, *, id: str | None = None, name: str | None = None) -> Evaluator | N Returns ------- Optional[Evaluator]: The metric if found (SplunkAOEvaluator, LlmEvaluator, CodeEvaluator, or DecisionEvaluator), - None otherwise. + None otherwise. ``None`` can also mean the evaluator exists but isn't shared with the caller's projects. Raises ------ @@ -384,7 +390,9 @@ def delete_by_name(cls, name: str) -> None: Raises ------ - ValueError: If no metric with the given name exists. + ValueError: If no metric with the given name exists, or it isn't shared with your projects. + EvaluatorPermissionError: If the API answers 403 (owner or editor role needed on one of the + evaluator's projects; org admin for a global evaluator). Examples -------- @@ -514,6 +522,8 @@ def update(self, **kwargs: Any) -> Evaluator: or the metric is in FAILED_SYNC state. ValueError: If any unsupported fields are passed. APIError: If the API returns a validation error or empty response. + EvaluatorPermissionError: If the API answers 403: updating needs the owner or editor role on one of the + evaluator's projects (org admin for a global evaluator). State is set to FAILED_SYNC. Exception: If the API call fails (state is set to FAILED_SYNC). Examples @@ -546,7 +556,8 @@ def update(self, **kwargs: Any) -> Evaluator: logger.info(f"Evaluator.update: id='{self.id}' name='{self.name}' - started") try: config = SplunkAOConfig.get() - response = update_scorers_scorer_id_patch.sync(scorer_id=self.id, client=config.api_client, body=body) + with _translate_forbidden(_edit_denied_hint("update")): + response = update_scorers_scorer_id_patch.sync(scorer_id=self.id, client=config.api_client, body=body) except Exception as e: self._set_state(SyncState.FAILED_SYNC, error=e) logger.error(f"Evaluator.update: id='{self.id}' - failed: {e}") @@ -562,6 +573,90 @@ def update(self, **kwargs: Any) -> Evaluator: logger.info(f"Evaluator.update: id='{self.id}' - completed") return self + def set_scope(self, *, project_ids: builtins.list[str] | None = None, is_global: bool = False) -> Evaluator: + """ + Replace this metric's scope on the API. + + This is a full replace: the metric ends up scoped to exactly ``project_ids``, or global. It never + falls back to the current project. On success the instance is updated with the API response and + returned in SYNCED state. + + Parameters + ---------- + project_ids : list[str], optional + Projects to scope the metric to. Required (and non-empty) unless ``is_global=True``. + is_global : bool + Make the metric global. Cannot be combined with ``project_ids``. Org admins only. + + Returns + ------- + Evaluator: This metric instance with updated attributes from the API. + + Raises + ------ + ValidationError: If this is a local metric, or the arguments are invalid (``is_global=True`` with + ``project_ids``, ``project_ids`` missing, empty or a single string). + ValueError: If the metric ID is not set, the metric is deleted, or the metric is in FAILED_SYNC state. + APIError: If the API returns a validation error or empty response. + EvaluatorPermissionError: If the API answers 403: the owner or editor role is needed on each project + added or removed, and the org admin role to make the metric global. + NotFoundError: If the API answers 404. The server also answers 404 for a metric that exists but + isn't shared with your projects. + Exception: If the API call fails (state is set to FAILED_SYNC). + + Examples + -------- + metric = Evaluator.get(name="factuality-checker") + metric.set_scope(project_ids=[""]) + metric.set_scope(is_global=True) # org admins only + """ + if isinstance(self, LocalEvaluator): + raise ValidationError("Local metrics don't exist on the server and can't change scope.") + if self.id is None: + raise ValueError("Evaluator ID is not set. Cannot change the scope of a local-only metric.") + if self.sync_state == SyncState.DELETED: + raise ValueError("Cannot change the scope of a deleted metric.") + if self.sync_state == SyncState.FAILED_SYNC: + raise ValueError( + "Cannot change the scope of a metric in FAILED_SYNC state. " + "Call refresh() to re-sync from the API, then retry your changes." + ) + + # Typed as object so the str guard is not flagged unreachable for a list[str] parameter. + requested: object = project_ids + if isinstance(requested, str): + raise ValidationError("project_ids must be a list of project ids, not a single string.") + if is_global: + if project_ids: + raise ValidationError("A metric cannot be both global and scoped: pass is_global=True or project_ids.") + body = UpdateScorerScopeRequest(is_global=True) + else: + if not project_ids: + raise ValidationError("project_ids must not be empty. Pass is_global=True to make the metric global.") + body = UpdateScorerScopeRequest(is_global=False, project_ids=list(dict.fromkeys(project_ids))) + + logger.info(f"Evaluator.set_scope: id='{self.id}' name='{self.name}' - started") + try: + config = SplunkAOConfig.get() + with _translate_forbidden(_SCOPE_CHANGE_DENIED_HINT): + response = set_scorer_scope_scorers_scorer_id_scope_put.sync( + scorer_id=self.id, client=config.api_client, body=body + ) + except Exception as e: + self._set_state(SyncState.FAILED_SYNC, error=e) + logger.error(f"Evaluator.set_scope: id='{self.id}' - failed: {e}") + raise + + if isinstance(response, HTTPValidationError): + raise APIError(f"Evaluator scope validation error: {response.detail}") + if response is None: + raise APIError(f"Evaluator scope update returned empty response for id '{self.id}'") + + self._populate_from_scorer_response(response) + self._set_state(SyncState.SYNCED) + logger.info(f"Evaluator.set_scope: id='{self.id}' - completed") + return self + def delete(self) -> None: """ Delete this metric. @@ -571,7 +666,9 @@ def delete(self) -> None: Raises ------ ValidationError: If this is a local metric. - ValueError: If the metric is not synced. + ValueError: If the metric is not synced, or is not found (or not shared with your projects). + EvaluatorPermissionError: If the API answers 403: deleting needs the owner or editor role on one of the + evaluator's projects (org admin for a global evaluator). Examples -------- @@ -605,7 +702,8 @@ def refresh(self) -> None: ------ ValidationError: If this is a local metric. ValueError: If the metric is not synced. - Exception: If the API call fails or the metric no longer exists. + ValueError: If the metric is not found, or not shared with your projects. + Exception: If the API call fails. Examples -------- @@ -625,7 +723,7 @@ def refresh(self) -> None: retrieved_scorer = next((s for s in scorers if s.id == self.id), None) if retrieved_scorer is None: - raise ValueError(f"Evaluator with id '{self.id}' no longer exists") + raise ValueError(f"Evaluator with id '{self.id}' not found, or not shared with your projects") self._populate_from_scorer_response(retrieved_scorer) self._set_state(SyncState.SYNCED) @@ -847,6 +945,8 @@ def create(self) -> LlmEvaluator: ValidationError: If configuration is invalid, or the scope is invalid (``is_global=True`` with ``project_ids``, an empty ``project_ids``, or ``is_global=False`` with no project). Scope errors are ``ValidationError``, not ``ValueError``. + EvaluatorPermissionError: If the API answers 403: org admins only for a global evaluator, owner or + editor role on the projects for a scoped one. NotFoundError: If the current project is set but cannot be found. Exception: If the API call fails. @@ -1020,6 +1120,8 @@ def create(self) -> DecisionEvaluator: ValidationError: If the scope is invalid (``is_global=True`` with ``project_ids``, an empty ``project_ids``, or ``is_global=False`` with no project). Scope errors are ``ValidationError``, not ``ValueError``. + EvaluatorPermissionError: If the API answers 403: org admins only for a global evaluator, owner or + editor role on the projects for a scoped one. NotFoundError: If the current project is set but cannot be found. ValueError: If an argument is invalid or the API answers with a validation error. SplunkAOAPIError: For HTTP errors. A scorer created before a clear rejection is deleted. @@ -1361,6 +1463,8 @@ def create(self) -> CodeEvaluator: ValidationError: If code is not set, validation fails, or configuration is invalid, or the scope is invalid (``is_global=True`` with ``project_ids``, an empty ``project_ids``, or ``is_global=False`` with no project). Scope errors are ``ValidationError``, not ``ValueError``. + EvaluatorPermissionError: If the API answers 403: org admins only for a global evaluator, owner or + editor role on the projects for a scoped one. NotFoundError: If the current project is set but cannot be found. Exception: If the API call fails. @@ -1416,8 +1520,11 @@ def create(self) -> CodeEvaluator: project_ids=scope_project_ids, ) - scorer_response = create_scorers_post.sync(client=config.api_client, body=scorer_request) + with _translate_forbidden(_create_denied_hint(scope_is_global)): + scorer_response = create_scorers_post.sync(client=config.api_client, body=scorer_request) + if isinstance(scorer_response, HTTPValidationError): + raise ValueError(scorer_response.detail) if scorer_response is None: logger.debug("CodeEvaluator.create: No response from create_scorers_post") raise ValueError("Failed to create code-based metric: No response from API") diff --git a/src/splunk_ao/evaluators.py b/src/splunk_ao/evaluators.py index 512da5eb..8ed0ab2e 100644 --- a/src/splunk_ao/evaluators.py +++ b/src/splunk_ao/evaluators.py @@ -1,11 +1,12 @@ import datetime import logging -from collections.abc import Mapping, Sequence +from collections.abc import Iterator, Mapping, Sequence +from contextlib import contextmanager from typing import Any from galileo_core.schemas.logging.step import StepType from splunk_ao.config import SplunkAOConfig -from splunk_ao.exceptions import SplunkAOAPIError +from splunk_ao.exceptions import EvaluatorPermissionError, ForbiddenError, SplunkAOAPIError from splunk_ao.resources.api.data import ( create_decision_scorer_version_scorers_scorer_id_version_decision_post, create_llm_scorer_version_scorers_scorer_id_version_llm_post, @@ -94,6 +95,52 @@ def __init__(self, scorer_id: str, cause: Exception) -> None: ) +_GLOBAL_CREATE_DENIED_HINT = ( + "Only org admins can create global evaluators. Pass project_ids=[...] to scope the evaluator to your projects, " + "or set SPLUNK_AO_PROJECT / SPLUNK_AO_PROJECT_ID so it defaults to one." +) +_SCOPED_CREATE_DENIED_HINT = ( + "Creating a project-scoped evaluator needs the owner or editor role on every project in project_ids " + "(or on the current project when it is the default). Ask a project owner for access, or choose other projects." +) +_SCOPE_CHANGE_DENIED_HINT = ( + "Changing an evaluator's scope needs the owner or editor role on each project added or removed; " + "making it global needs the org admin role." +) + + +def _edit_denied_hint(action: str) -> str: + """Hint for a 403 when updating or deleting an evaluator.""" + return ( + f"You can't {action} this evaluator. It needs the owner or editor role on one of the evaluator's projects " + "(org admin for a global evaluator)." + ) + + +def _create_denied_hint(scope_is_global: bool | Unset) -> str: + """Hint for a 403 on create, chosen from the scope that was actually sent.""" + return _SCOPED_CREATE_DENIED_HINT if scope_is_global is False else _GLOBAL_CREATE_DENIED_HINT + + +def _as_permission_error(error: ForbiddenError, hint: str) -> EvaluatorPermissionError: + """Turn a generic 403 into an ``EvaluatorPermissionError``; an existing one is returned unchanged.""" + if isinstance(error, EvaluatorPermissionError): + return error + return EvaluatorPermissionError(error.status_code, error.content, hint) + + +@contextmanager +def _translate_forbidden(hint: str) -> Iterator[None]: + """Re-raise a 403 from the wrapped call as an ``EvaluatorPermissionError`` carrying ``hint``.""" + try: + yield + except ForbiddenError as error: + translated = _as_permission_error(error, hint) + if translated is error: + raise + raise translated from error + + REJECTION_STATUS_CODES = frozenset({400, 403, 404, 422}) @@ -261,10 +308,13 @@ def __init__(self) -> None: def delete_evaluator(self, name: str) -> None: scorers_to_delete = Scorers().list(name=name) if not scorers_to_delete: - raise ValueError(f"Scorer with name {name} not found.") + raise ValueError(f"Evaluator with name '{name}' not found, or not shared with your projects.") for scorer in scorers_to_delete: - response = delete_scorer_scorers_scorer_id_delete.sync(scorer_id=scorer.id, client=self.config.api_client) + with _translate_forbidden(_edit_denied_hint("delete")): + response = delete_scorer_scorers_scorer_id_delete.sync( + scorer_id=scorer.id, client=self.config.api_client + ) if isinstance(response, HTTPValidationError): raise ValueError(response.detail) @@ -329,6 +379,9 @@ def create_custom_llm_evaluator( ValidationError If the scope arguments conflict, ``project_ids`` is empty, or ``is_global=False`` is passed with no project to scope to. + EvaluatorPermissionError + If the API answers 403: org admins only for a global evaluator, owner or editor role on the + projects for a scoped one. NotFoundError If the current project is set but cannot be found. """ @@ -348,7 +401,12 @@ def create_custom_llm_evaluator( project_ids=scope_project_ids, ) - scorer = create_scorers_post.sync(body=create_scorer_request, client=self.config.api_client) + with _translate_forbidden(_create_denied_hint(scope_is_global)): + scorer = create_scorers_post.sync(body=create_scorer_request, client=self.config.api_client) + if isinstance(scorer, HTTPValidationError): + raise ValueError(scorer.detail) + if scorer is None: + raise ValueError("Failed to create custom LLM evaluator.") version_req = CreateLLMScorerVersionRequest(user_prompt=user_prompt) version_resp = create_llm_scorer_version_scorers_scorer_id_version_llm_post.sync( @@ -420,6 +478,9 @@ def create_decision_evaluator( ValidationError If the scope arguments conflict, ``project_ids`` is empty, or ``is_global=False`` is passed with no project to scope to. + EvaluatorPermissionError + If the API answers 403: org admins only for a global evaluator, owner or editor role on the + projects for a scoped one. NotFoundError If the current project is set but cannot be found. ValueError @@ -452,7 +513,9 @@ def create_decision_evaluator( project_ids=scope_project_ids, ) - scorer = create_scorers_post.sync(body=create_scorer_request, client=self.config.api_client) + create_hint = _create_denied_hint(scope_is_global) + with _translate_forbidden(create_hint): + scorer = create_scorers_post.sync(body=create_scorer_request, client=self.config.api_client) if isinstance(scorer, HTTPValidationError): raise ValueError(scorer.detail) if scorer is None: @@ -469,6 +532,10 @@ def create_decision_evaluator( except Exception as error: if self._was_rejected(error): self._delete_scorer_quietly(scorer.id) + if isinstance(error, ForbiddenError): + translated = _as_permission_error(error, create_hint) + if translated is not error: + raise translated from error raise _logger.warning( "Decision evaluator version request failed with an unknown outcome; scorer_id=%s was kept: %s", @@ -599,6 +666,9 @@ def create_custom_llm_evaluator( ValidationError If the scope arguments conflict, ``project_ids`` is empty, or ``is_global=False`` is passed with no project to scope to. + EvaluatorPermissionError + If the API answers 403: org admins only for a global evaluator, owner or editor role on the + projects for a scoped one. NotFoundError If the current project is set but cannot be found. """ @@ -683,6 +753,9 @@ def create_decision_evaluator( ValidationError If the scope arguments conflict, ``project_ids`` is empty, or ``is_global=False`` is passed with no project to scope to. + EvaluatorPermissionError + If the API answers 403: org admins only for a global evaluator, owner or editor role on the + projects for a scoped one. NotFoundError If the current project is set but cannot be found. ValueError diff --git a/src/splunk_ao/exceptions.py b/src/splunk_ao/exceptions.py index 8b1af634..a61e729c 100644 --- a/src/splunk_ao/exceptions.py +++ b/src/splunk_ao/exceptions.py @@ -6,6 +6,7 @@ "AuthenticationError", "BadRequestError", "ConflictError", + "EvaluatorPermissionError", "ForbiddenError", "NotFoundError", "RateLimitError", @@ -65,6 +66,17 @@ def __init__(self, status_code: int, content: bytes): ) +class EvaluatorPermissionError(ForbiddenError): + """HTTP 403 on an evaluator create, update, delete or scope change. + + Subclasses ``ForbiddenError``, so existing ``except ForbiddenError`` handlers still catch it. The message says + which role or permission the operation needs; the server response body follows it. + """ + + def __init__(self, status_code: int, content: bytes, message: str): + SplunkAOAPIError.__init__(self, status_code, content, message) + + class NotFoundError(SplunkAOAPIError): r"""HTTP 404 - Resource not found. diff --git a/tests/test_evaluator.py b/tests/test_evaluator.py index 49301811..251105f7 100644 --- a/tests/test_evaluator.py +++ b/tests/test_evaluator.py @@ -635,7 +635,7 @@ def test_refresh_raises_error_when_metric_no_longer_exists( metric = Evaluator.get(id=metric_id) - with pytest.raises(ValueError, match="no longer exists"): + with pytest.raises(ValueError, match="not found, or not shared with your projects"): metric.refresh() diff --git a/tests/test_evaluator_permissions.py b/tests/test_evaluator_permissions.py new file mode 100644 index 00000000..7ad20c0a --- /dev/null +++ b/tests/test_evaluator_permissions.py @@ -0,0 +1,453 @@ +"""Clear errors for evaluator permission denials, and ``Evaluator.set_scope``.""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import MagicMock, patch +from uuid import uuid4 + +import pytest + +import splunk_ao +from splunk_ao.evaluator import CodeEvaluator, DecisionEvaluator, Evaluator, LlmEvaluator, LocalEvaluator +from splunk_ao.evaluators import Evaluators, create_custom_llm_evaluator, create_decision_evaluator +from splunk_ao.exceptions import EvaluatorPermissionError, ForbiddenError, NotFoundError +from splunk_ao.resources.models import ( + HTTPValidationError, + ScorerResponse, + ScorerScopeProjectRef, + UpdateScorerScopeRequest, +) +from splunk_ao.resources.types import UNSET +from splunk_ao.shared.base import SyncState +from splunk_ao.shared.exceptions import APIError, ValidationError + +pytestmark = pytest.mark.usefixtures("no_current_project") + +SERVER_BODY = b'{"detail": "denied by server"}' +GLOBAL_HINT = "Only org admins can create global evaluators" +SCOPED_HINT = "owner or editor role on every project in project_ids" +EDIT_HINT = "owner or editor role on one of the evaluator's projects" +SCOPE_HINT = "owner or editor role on each project added or removed" + + +def _forbidden() -> ForbiddenError: + return ForbiddenError(403, SERVER_BODY) + + +def _assert_permission_error(error: BaseException, hint: str) -> None: + assert isinstance(error, EvaluatorPermissionError) + assert isinstance(error, ForbiddenError) + assert error.status_code == 403 + assert hint in error.message + assert str(error).startswith(error.message) + assert "denied by server" in str(error) + + +def _scorer_response(scorer_id: str, **extra: Any) -> ScorerResponse: + row: dict[str, Any] = { + "id": scorer_id, + "name": "ev", + "scorer_type": "llm", + "tags": [], + "created_at": "2026-01-01T00:00:00+00:00", + "updated_at": "2026-01-01T00:00:00+00:00", + } + row.update(extra) + return ScorerResponse.from_dict(row) + + +def _synced(evaluator: Evaluator) -> Evaluator: + evaluator._sync_attrs(id=str(uuid4())) + evaluator._set_state(SyncState.SYNCED) + return evaluator + + +class TestEvaluatorPermissionErrorClass: + def test_is_forbidden_error_and_exported(self) -> None: + # Given/When: the error built the way the SDK builds it + error = EvaluatorPermissionError(403, SERVER_BODY, "Only org admins can do this.") + + # Then: it is a ForbiddenError with the hint first and the server body after it + assert isinstance(error, ForbiddenError) + assert error.status_code == 403 + assert str(error).startswith("Only org admins can do this. (HTTP 403)") + assert "denied by server" in str(error) + assert "EvaluatorPermissionError" in splunk_ao.exceptions.__all__ + assert splunk_ao.EvaluatorPermissionError is EvaluatorPermissionError + assert "EvaluatorPermissionError" in splunk_ao.__all__ + + +@patch("splunk_ao.evaluators.create_llm_scorer_version_scorers_scorer_id_version_llm_post") +@patch("splunk_ao.evaluators.create_scorers_post") +class TestCreateLlmDenied: + @pytest.mark.parametrize("use_function", [False, True], ids=["method", "module_function"]) + @pytest.mark.parametrize("kwargs", [{}, {"is_global": True}], ids=["unset_scope", "explicit_global"]) + def test_global_create_403_names_admins_and_the_fix( + self, mock_create_scorer, mock_create_version, use_function, kwargs + ) -> None: + # Given: the server refuses the create + mock_create_scorer.sync.side_effect = _forbidden() + + # When/Then: the error says only org admins can create global evaluators and how to scope + with pytest.raises(EvaluatorPermissionError) as exc_info: + if use_function: + create_custom_llm_evaluator("m", "prompt", **kwargs) + else: + Evaluators().create_custom_llm_evaluator(name="m", user_prompt="prompt", **kwargs) + _assert_permission_error(exc_info.value, GLOBAL_HINT) + assert "project_ids=[...]" in str(exc_info.value) + assert "SPLUNK_AO_PROJECT / SPLUNK_AO_PROJECT_ID" in str(exc_info.value) + mock_create_version.sync.assert_not_called() + + @pytest.mark.parametrize("use_function", [False, True], ids=["method", "module_function"]) + def test_scoped_create_403_names_the_project_role( + self, mock_create_scorer, mock_create_version, use_function + ) -> None: + # Given: a scoped create the server refuses + mock_create_scorer.sync.side_effect = _forbidden() + + # When/Then: the message is about the project role, not about admins + with pytest.raises(EvaluatorPermissionError) as exc_info: + if use_function: + create_custom_llm_evaluator("m", "prompt", project_ids=["p1"]) + else: + Evaluators().create_custom_llm_evaluator(name="m", user_prompt="prompt", project_ids=["p1"]) + _assert_permission_error(exc_info.value, SCOPED_HINT) + assert GLOBAL_HINT not in str(exc_info.value) + + def test_hint_follows_the_resolved_scope_not_the_instance( + self, mock_create_scorer, mock_create_version, monkeypatch + ) -> None: + # Given: no explicit scope, but a current project that resolves the scope to that project + monkeypatch.setenv("SPLUNK_AO_PROJECT_ID", "proj-1") + mock_create_scorer.sync.side_effect = _forbidden() + + # When/Then: the scoped hint is used because the resolved scope is not global + with patch("splunk_ao.evaluators._resolve_project", return_value=MagicMock(id="proj-1")): + with pytest.raises(EvaluatorPermissionError) as exc_info: + Evaluators().create_custom_llm_evaluator(name="m", user_prompt="prompt") + _assert_permission_error(exc_info.value, SCOPED_HINT) + + def test_already_translated_error_is_not_wrapped_again(self, mock_create_scorer, mock_create_version) -> None: + # Given: a permission error with its own hint raised from the endpoint + original = EvaluatorPermissionError(403, b"x", "Custom hint.") + mock_create_scorer.sync.side_effect = original + + # When/Then: the same object comes out + with pytest.raises(EvaluatorPermissionError) as exc_info: + Evaluators().create_custom_llm_evaluator(name="m", user_prompt="prompt", is_global=True) + assert exc_info.value is original + + def test_422_is_raised_as_value_error(self, mock_create_scorer, mock_create_version) -> None: + # Given: the scorer post answers with a validation error + mock_create_scorer.sync.return_value = HTTPValidationError(detail="bad name") + + # When/Then: the detail surfaces and no version request is made + with pytest.raises(ValueError, match="bad name"): + Evaluators().create_custom_llm_evaluator(name="m", user_prompt="prompt", is_global=True) + mock_create_version.sync.assert_not_called() + + def test_empty_response_is_raised_as_value_error(self, mock_create_scorer, mock_create_version) -> None: + mock_create_scorer.sync.return_value = None + with pytest.raises(ValueError, match="Failed to create custom LLM evaluator"): + Evaluators().create_custom_llm_evaluator(name="m", user_prompt="prompt", is_global=True) + + def test_llm_evaluator_create_keeps_the_error_and_marks_failed( + self, mock_create_scorer, mock_create_version + ) -> None: + # Given: an LlmEvaluator with the default (global) scope the server refuses + mock_create_scorer.sync.side_effect = _forbidden() + evaluator = LlmEvaluator(name="m", prompt="p") + + # When/Then: the permission error is raised once, and the state is FAILED_SYNC + with pytest.raises(EvaluatorPermissionError) as exc_info: + evaluator.create() + _assert_permission_error(exc_info.value, GLOBAL_HINT) + assert exc_info.value.__cause__ is not None + assert type(exc_info.value.__cause__) is ForbiddenError + assert evaluator.sync_state == SyncState.FAILED_SYNC + + def test_llm_evaluator_create_with_projects_says_owner_or_editor( + self, mock_create_scorer, mock_create_version + ) -> None: + mock_create_scorer.sync.side_effect = _forbidden() + with pytest.raises(EvaluatorPermissionError) as exc_info: + LlmEvaluator(name="m", prompt="p", project_ids=["p1"]).create() + _assert_permission_error(exc_info.value, SCOPED_HINT) + + +@patch("splunk_ao.evaluators.delete_scorer_scorers_scorer_id_delete") +@patch("splunk_ao.evaluators.create_decision_scorer_version_scorers_scorer_id_version_decision_post") +@patch("splunk_ao.evaluators.create_scorers_post") +class TestCreateDecisionDenied: + @pytest.mark.parametrize("use_function", [False, True], ids=["method", "module_function"]) + @pytest.mark.parametrize( + ("kwargs", "hint"), + [({}, GLOBAL_HINT), ({"is_global": True}, GLOBAL_HINT), ({"project_ids": ["p1"]}, SCOPED_HINT)], + ids=["unset_scope", "explicit_global", "scoped"], + ) + def test_scorer_post_403_is_translated( + self, mock_create_scorer, mock_create_version, mock_delete, use_function, kwargs, hint + ) -> None: + # Given: the scorer post is refused + mock_create_scorer.sync.side_effect = _forbidden() + create = create_decision_evaluator if use_function else Evaluators().create_decision_evaluator + + # When/Then: the hint matches the resolved scope; nothing else was called + with pytest.raises(EvaluatorPermissionError) as exc_info: + create(name="tone", question="q", **kwargs) + _assert_permission_error(exc_info.value, hint) + mock_create_version.sync.assert_not_called() + mock_delete.sync.assert_not_called() + + def test_version_post_403_is_translated_and_the_scorer_is_still_deleted( + self, mock_create_scorer, mock_create_version, mock_delete + ) -> None: + # Given: the scorer is created, then the version post is refused + scorer = _scorer_response(str(uuid4()), scorer_type="decision") + mock_create_scorer.sync.return_value = scorer + mock_create_version.sync.side_effect = _forbidden() + + # When/Then: the permission error carries the scoped hint and the orphan scorer is deleted + with pytest.raises(EvaluatorPermissionError) as exc_info: + create_decision_evaluator(name="tone", question="q", project_ids=["p1"]) + _assert_permission_error(exc_info.value, SCOPED_HINT) + mock_delete.sync.assert_called_once() + assert mock_delete.sync.call_args.kwargs["scorer_id"] == scorer.id + + def test_decision_evaluator_create_raises_one_permission_error( + self, mock_create_scorer, mock_create_version, mock_delete + ) -> None: + mock_create_scorer.sync.side_effect = _forbidden() + evaluator = DecisionEvaluator(name="tone", question="q") + with pytest.raises(EvaluatorPermissionError) as exc_info: + evaluator.create() + _assert_permission_error(exc_info.value, GLOBAL_HINT) + assert type(exc_info.value.__cause__) is ForbiddenError + assert evaluator.sync_state == SyncState.FAILED_SYNC + + +@patch("splunk_ao.evaluator.SplunkAOConfig.get") +@patch("splunk_ao.evaluator.create_scorers_post") +class TestCreateCodeDenied: + @pytest.mark.parametrize( + ("kwargs", "hint"), + [({}, GLOBAL_HINT), ({"is_global": True}, GLOBAL_HINT), ({"project_ids": ["p1"]}, SCOPED_HINT)], + ids=["unset_scope", "explicit_global", "scoped"], + ) + def test_403_is_translated_by_resolved_scope(self, mock_create_scorers, mock_config, kwargs, hint) -> None: + # Given: code validation passes and the scorer post is refused + mock_create_scorers.sync.side_effect = _forbidden() + evaluator = CodeEvaluator(name="c", code="def f(): pass", **kwargs) + + # When/Then + with patch.object(CodeEvaluator, "_validate_code", return_value="{}"): + with pytest.raises(EvaluatorPermissionError) as exc_info: + evaluator.create() + _assert_permission_error(exc_info.value, hint) + assert type(exc_info.value.__cause__) is ForbiddenError + assert evaluator.sync_state == SyncState.FAILED_SYNC + + def test_422_is_raised_as_value_error(self, mock_create_scorers, mock_config) -> None: + # Given: the scorer post answers with a validation error + mock_create_scorers.sync.return_value = HTTPValidationError(detail="bad code scorer") + + # When/Then: the detail surfaces instead of an AttributeError on .id + with patch.object(CodeEvaluator, "_validate_code", return_value="{}"): + with pytest.raises(ValueError, match="bad code scorer"): + CodeEvaluator(name="c", code="x", is_global=True).create() + + +class TestUpdateAndDeleteDenied: + @patch("splunk_ao.evaluator.update_scorers_scorer_id_patch") + @patch("splunk_ao.evaluator.SplunkAOConfig.get") + def test_update_403_is_translated_and_marks_failed(self, mock_config, mock_patch) -> None: + # Given: a synced evaluator the caller cannot edit + evaluator = _synced(LlmEvaluator(name="m", prompt="p")) + mock_patch.sync.side_effect = _forbidden() + + # When/Then + with pytest.raises(EvaluatorPermissionError) as exc_info: + evaluator.update(name="new") + _assert_permission_error(exc_info.value, EDIT_HINT) + assert "org admin for a global evaluator" in str(exc_info.value) + assert evaluator.sync_state == SyncState.FAILED_SYNC + + @patch("splunk_ao.evaluator.update_scorers_scorer_id_patch") + @patch("splunk_ao.evaluator.SplunkAOConfig.get") + def test_update_404_stays_not_found(self, mock_config, mock_patch) -> None: + evaluator = _synced(LlmEvaluator(name="m", prompt="p")) + mock_patch.sync.side_effect = NotFoundError(404, b"gone") + with pytest.raises(NotFoundError) as exc_info: + evaluator.update(name="new") + assert not isinstance(exc_info.value, EvaluatorPermissionError) + assert evaluator.sync_state == SyncState.FAILED_SYNC + + @patch("splunk_ao.evaluators.delete_scorer_scorers_scorer_id_delete") + @patch("splunk_ao.scorers.Scorers.list") + def test_delete_evaluator_403_is_translated(self, mock_list, mock_delete) -> None: + mock_list.return_value = [_scorer_response(str(uuid4()))] + mock_delete.sync.side_effect = _forbidden() + with pytest.raises(EvaluatorPermissionError) as exc_info: + Evaluators().delete_evaluator(name="ev") + _assert_permission_error(exc_info.value, EDIT_HINT) + + @patch("splunk_ao.evaluators.delete_scorer_scorers_scorer_id_delete") + @patch("splunk_ao.scorers.Scorers.list") + def test_delete_by_name_403_is_translated(self, mock_list, mock_delete) -> None: + mock_list.return_value = [_scorer_response(str(uuid4()))] + mock_delete.sync.side_effect = _forbidden() + with pytest.raises(EvaluatorPermissionError) as exc_info: + Evaluator.delete_by_name("ev") + _assert_permission_error(exc_info.value, EDIT_HINT) + + @patch("splunk_ao.evaluators.delete_scorer_scorers_scorer_id_delete") + @patch("splunk_ao.scorers.Scorers.list") + def test_instance_delete_403_is_translated_and_marks_failed(self, mock_list, mock_delete) -> None: + evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) + mock_list.return_value = [_scorer_response(str(uuid4()))] + mock_delete.sync.side_effect = _forbidden() + with pytest.raises(EvaluatorPermissionError) as exc_info: + evaluator.delete() + _assert_permission_error(exc_info.value, EDIT_HINT) + assert evaluator.sync_state == SyncState.FAILED_SYNC + + @patch("splunk_ao.evaluators.delete_scorer_scorers_scorer_id_delete") + @patch("splunk_ao.scorers.Scorers.list") + def test_delete_404_stays_not_found(self, mock_list, mock_delete) -> None: + mock_list.return_value = [_scorer_response(str(uuid4()))] + mock_delete.sync.side_effect = NotFoundError(404, b"gone") + with pytest.raises(NotFoundError) as exc_info: + Evaluators().delete_evaluator(name="ev") + assert not isinstance(exc_info.value, EvaluatorPermissionError) + + +class TestNotFoundWording: + @patch("splunk_ao.scorers.Scorers.list", return_value=[]) + def test_delete_evaluator_name_not_in_list(self, mock_list) -> None: + with pytest.raises(ValueError, match="not found, or not shared with your projects"): + Evaluators().delete_evaluator(name="ghost") + + @patch("splunk_ao.evaluator.Scorers") + def test_refresh_id_not_in_list(self, mock_scorers) -> None: + evaluator = _synced(LlmEvaluator(name="m", prompt="p")) + mock_scorers.return_value.list.return_value = [] + with pytest.raises(ValueError, match="not found, or not shared with your projects"): + evaluator.refresh() + + @patch("splunk_ao.evaluator.Scorers") + def test_get_still_returns_none_and_documents_why(self, mock_scorers) -> None: + mock_scorers.return_value.list.return_value = [] + assert Evaluator.get(name="ghost") is None + assert Evaluator.get(id="ghost-id") is None + assert "isn't shared with the caller's projects" in (Evaluator.get.__doc__ or "") + + +@patch("splunk_ao.evaluator.set_scorer_scope_scorers_scorer_id_scope_put") +@patch("splunk_ao.evaluator.SplunkAOConfig.get") +class TestSetScope: + def test_scoped_request_populates_instance_and_returns_self(self, mock_config, mock_put) -> None: + # Given: a synced evaluator created with its own (private) scope request + evaluator = _synced(LlmEvaluator(name="ev", prompt="p", project_ids=["old"])) + mock_put.sync.return_value = _scorer_response( + evaluator.id, is_global=False, scope_projects=[{"id": "a", "name": "A"}, {"id": "b", "name": "B"}] + ) + + # When + result = evaluator.set_scope(project_ids=["a", "b", "a"]) + + # Then: the full-replace request is deduplicated, the response is applied, the create-time scope is untouched + assert result is evaluator + kwargs = mock_put.sync.call_args.kwargs + assert kwargs["scorer_id"] == evaluator.id + assert kwargs["client"] is mock_config.return_value.api_client + body = kwargs["body"] + assert isinstance(body, UpdateScorerScopeRequest) + assert body.is_global is False + assert body.project_ids == ["a", "b"] + assert evaluator.is_global is False + assert [p.id for p in evaluator.scope_projects] == ["a", "b"] + assert all(isinstance(p, ScorerScopeProjectRef) for p in evaluator.scope_projects) + assert evaluator.is_synced() + assert evaluator._create_project_ids == ["old"] + assert evaluator._create_is_global is None + + def test_global_request_sends_no_project_ids(self, mock_config, mock_put) -> None: + evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) + mock_put.sync.return_value = _scorer_response(evaluator.id, is_global=True, scope_projects=[]) + + evaluator.set_scope(is_global=True) + + body = mock_put.sync.call_args.kwargs["body"] + assert body.is_global is True + assert body.project_ids is UNSET + assert "project_ids" not in body.to_dict() + assert evaluator.is_global is True + assert evaluator.is_synced() + + @pytest.mark.parametrize( + ("kwargs", "message"), + [ + ({"is_global": True, "project_ids": ["a"]}, "both global and scoped"), + ({"is_global": False}, "must not be empty"), + ({"project_ids": []}, "must not be empty"), + ({"project_ids": None, "is_global": False}, "must not be empty"), + ({"project_ids": "a-single-id"}, "not a single string"), + ({"project_ids": "a-single-id", "is_global": True}, "not a single string"), + ], + ) + def test_invalid_arguments_make_no_request_and_keep_state(self, mock_config, mock_put, kwargs, message) -> None: + evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) + with pytest.raises(ValidationError, match=message): + evaluator.set_scope(**kwargs) + mock_put.sync.assert_not_called() + assert evaluator.is_synced() + + def test_local_evaluator_is_rejected(self, mock_config, mock_put) -> None: + with pytest.raises(ValidationError, match="Local metrics"): + LocalEvaluator(name="local", scorer_fn=lambda trace: 1.0).set_scope(project_ids=["a"]) + mock_put.sync.assert_not_called() + + def test_missing_id_is_rejected(self, mock_config, mock_put) -> None: + with pytest.raises(ValueError, match="ID is not set"): + LlmEvaluator(name="ev", prompt="p").set_scope(project_ids=["a"]) + mock_put.sync.assert_not_called() + + @pytest.mark.parametrize("state", [SyncState.DELETED, SyncState.FAILED_SYNC]) + def test_deleted_and_failed_sync_are_rejected(self, mock_config, mock_put, state) -> None: + evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) + evaluator._set_state(state) + with pytest.raises(ValueError, match=r"deleted|FAILED_SYNC"): + evaluator.set_scope(project_ids=["a"]) + mock_put.sync.assert_not_called() + + @pytest.mark.parametrize("kwargs", [{"project_ids": ["a"]}, {"is_global": True}], ids=["scoped", "global"]) + def test_403_is_translated_and_marks_failed(self, mock_config, mock_put, kwargs) -> None: + evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) + mock_put.sync.side_effect = _forbidden() + with pytest.raises(EvaluatorPermissionError) as exc_info: + evaluator.set_scope(**kwargs) + _assert_permission_error(exc_info.value, SCOPE_HINT) + assert "making it global needs the org admin role" in str(exc_info.value) + assert evaluator.sync_state == SyncState.FAILED_SYNC + + def test_404_stays_not_found(self, mock_config, mock_put) -> None: + evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) + mock_put.sync.side_effect = NotFoundError(404, b"gone") + with pytest.raises(NotFoundError) as exc_info: + evaluator.set_scope(project_ids=["a"]) + assert not isinstance(exc_info.value, EvaluatorPermissionError) + assert evaluator.sync_state == SyncState.FAILED_SYNC + + def test_validation_error_response_raises_api_error(self, mock_config, mock_put) -> None: + evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) + mock_put.sync.return_value = HTTPValidationError(detail="unknown project") + with pytest.raises(APIError, match="unknown project"): + evaluator.set_scope(project_ids=["a"]) + + def test_empty_response_raises_api_error(self, mock_config, mock_put) -> None: + evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) + mock_put.sync.return_value = None + with pytest.raises(APIError, match="empty response"): + evaluator.set_scope(project_ids=["a"]) diff --git a/tests/test_evaluators.py b/tests/test_evaluators.py index d7cb067f..d6a65444 100644 --- a/tests/test_evaluators.py +++ b/tests/test_evaluators.py @@ -279,7 +279,7 @@ def test_delete_metric_not_found(self, mock_list_scorers) -> None: mock_list_scorers.return_value = [] metrics = Evaluators() - with pytest.raises(ValueError, match="Scorer with name test_metric not found."): + with pytest.raises(ValueError, match="Evaluator with name .test_metric. not found, or not shared with your projects"): metrics.delete_evaluator(name="test_metric") @patch("splunk_ao.evaluators.delete_scorer_scorers_scorer_id_delete") From 2ba47a1489d6481b1e2bd010253e5acf521385d2 Mon Sep 17 00:00:00 2001 From: quinn-galileo Date: Mon, 19 Oct 2026 12:37:39 -0400 Subject: [PATCH 2/9] fix: address self-review findings on permission errors Co-Authored-By: Claude Opus 5.5 (1M context) --- README.md | 3 +-- src/splunk_ao/evaluator.py | 8 ++++---- src/splunk_ao/evaluators.py | 21 +++++---------------- src/splunk_ao/exceptions.py | 3 ++- tests/test_evaluator_permissions.py | 2 +- tests/test_evaluators.py | 4 +++- 6 files changed, 16 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index 4c09a213..8eb930b6 100644 --- a/README.md +++ b/README.md @@ -634,8 +634,7 @@ New evaluators are scoped to the current project (`SPLUNK_AO_PROJECT_ID` or `SPL evaluators, and changing or deleting an evaluator needs the owner or editor role on one of its projects. ```python -from splunk_ao import EvaluatorPermissionError -from splunk_ao.__future__ import LlmEvaluator +from splunk_ao import EvaluatorPermissionError, LlmEvaluator try: evaluator = LlmEvaluator( diff --git a/src/splunk_ao/evaluator.py b/src/splunk_ao/evaluator.py index e2312186..2da16b2c 100644 --- a/src/splunk_ao/evaluator.py +++ b/src/splunk_ao/evaluator.py @@ -622,15 +622,15 @@ def set_scope(self, *, project_ids: builtins.list[str] | None = None, is_global: "Call refresh() to re-sync from the API, then retry your changes." ) - # Typed as object so the str guard is not flagged unreachable for a list[str] parameter. - requested: object = project_ids - if isinstance(requested, str): - raise ValidationError("project_ids must be a list of project ids, not a single string.") if is_global: if project_ids: raise ValidationError("A metric cannot be both global and scoped: pass is_global=True or project_ids.") body = UpdateScorerScopeRequest(is_global=True) else: + # Typed as object so the str guard is not flagged unreachable for a list[str] parameter. + requested: object = project_ids + if isinstance(requested, str): + raise ValidationError("project_ids must be a list of project ids, not a single string.") if not project_ids: raise ValidationError("project_ids must not be empty. Pass is_global=True to make the metric global.") body = UpdateScorerScopeRequest(is_global=False, project_ids=list(dict.fromkeys(project_ids))) diff --git a/src/splunk_ao/evaluators.py b/src/splunk_ao/evaluators.py index 8ed0ab2e..a92ee60b 100644 --- a/src/splunk_ao/evaluators.py +++ b/src/splunk_ao/evaluators.py @@ -122,23 +122,15 @@ def _create_denied_hint(scope_is_global: bool | Unset) -> str: return _SCOPED_CREATE_DENIED_HINT if scope_is_global is False else _GLOBAL_CREATE_DENIED_HINT -def _as_permission_error(error: ForbiddenError, hint: str) -> EvaluatorPermissionError: - """Turn a generic 403 into an ``EvaluatorPermissionError``; an existing one is returned unchanged.""" - if isinstance(error, EvaluatorPermissionError): - return error - return EvaluatorPermissionError(error.status_code, error.content, hint) - - @contextmanager def _translate_forbidden(hint: str) -> Iterator[None]: """Re-raise a 403 from the wrapped call as an ``EvaluatorPermissionError`` carrying ``hint``.""" try: yield + except EvaluatorPermissionError: + raise except ForbiddenError as error: - translated = _as_permission_error(error, hint) - if translated is error: - raise - raise translated from error + raise EvaluatorPermissionError(error.status_code, error.content, hint) from error REJECTION_STATUS_CODES = frozenset({400, 403, 404, 422}) @@ -532,11 +524,8 @@ def create_decision_evaluator( except Exception as error: if self._was_rejected(error): self._delete_scorer_quietly(scorer.id) - if isinstance(error, ForbiddenError): - translated = _as_permission_error(error, create_hint) - if translated is not error: - raise translated from error - raise + with _translate_forbidden(create_hint): + raise _logger.warning( "Decision evaluator version request failed with an unknown outcome; scorer_id=%s was kept: %s", scorer.id, diff --git a/src/splunk_ao/exceptions.py b/src/splunk_ao/exceptions.py index a61e729c..bba2ece4 100644 --- a/src/splunk_ao/exceptions.py +++ b/src/splunk_ao/exceptions.py @@ -73,7 +73,8 @@ class EvaluatorPermissionError(ForbiddenError): which role or permission the operation needs; the server response body follows it. """ - def __init__(self, status_code: int, content: bytes, message: str): + def __init__(self, status_code: int, content: bytes, message: str) -> None: + # ForbiddenError fixes its own message, so skip it and use the base initializer to set ours. SplunkAOAPIError.__init__(self, status_code, content, message) diff --git a/tests/test_evaluator_permissions.py b/tests/test_evaluator_permissions.py index 7ad20c0a..11de58b1 100644 --- a/tests/test_evaluator_permissions.py +++ b/tests/test_evaluator_permissions.py @@ -394,7 +394,7 @@ def test_global_request_sends_no_project_ids(self, mock_config, mock_put) -> Non ({"project_ids": []}, "must not be empty"), ({"project_ids": None, "is_global": False}, "must not be empty"), ({"project_ids": "a-single-id"}, "not a single string"), - ({"project_ids": "a-single-id", "is_global": True}, "not a single string"), + ({"project_ids": "a-single-id", "is_global": True}, "both global and scoped"), ], ) def test_invalid_arguments_make_no_request_and_keep_state(self, mock_config, mock_put, kwargs, message) -> None: diff --git a/tests/test_evaluators.py b/tests/test_evaluators.py index d6a65444..ede1f6c9 100644 --- a/tests/test_evaluators.py +++ b/tests/test_evaluators.py @@ -279,7 +279,9 @@ def test_delete_metric_not_found(self, mock_list_scorers) -> None: mock_list_scorers.return_value = [] metrics = Evaluators() - with pytest.raises(ValueError, match="Evaluator with name .test_metric. not found, or not shared with your projects"): + with pytest.raises( + ValueError, match="Evaluator with name .test_metric. not found, or not shared with your projects" + ): metrics.delete_evaluator(name="test_metric") @patch("splunk_ao.evaluators.delete_scorer_scorers_scorer_id_delete") From c61e456751bd6a28257ebbccad6c6b1f3d1f7710 Mon Sep 17 00:00:00 2001 From: quinn-galileo Date: Mon, 19 Oct 2026 12:39:32 -0400 Subject: [PATCH 3/9] fix: narrow changelog claim and tighten no-rewrap test Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 4 ++-- tests/test_evaluator_permissions.py | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cad0609f..06735208 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,8 +47,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `Evaluators.delete_evaluator()` and `Evaluator.refresh()` now say "not found, or not shared with your projects" when the evaluator is missing, since the server hides evaluators outside the caller's projects. `Evaluator.get()` still returns `None` in that case. A 404 from update, delete or `set_scope` stays a `NotFoundError`. -- Creating an LLM or code evaluator no longer fails with an `AttributeError` when the API answers with a validation - error; the detail is raised as a `ValueError`, as it already was for decision evaluators. +- Creating an LLM or code evaluator no longer fails with an `AttributeError` when the scorer request is answered with + a validation error; the detail is raised as a `ValueError`, as it already was for decision evaluators. - `SplunkAOTracingProcessor` (OpenAI Agents) now records Responses API LLM spans, the Agents SDK default, as chat messages: tool calls and tool results keep their own roles instead of one JSON `user` message, the output is a single assistant message, and workflow/agent spans show the diff --git a/tests/test_evaluator_permissions.py b/tests/test_evaluator_permissions.py index 11de58b1..55786646 100644 --- a/tests/test_evaluator_permissions.py +++ b/tests/test_evaluator_permissions.py @@ -138,6 +138,7 @@ def test_already_translated_error_is_not_wrapped_again(self, mock_create_scorer, with pytest.raises(EvaluatorPermissionError) as exc_info: Evaluators().create_custom_llm_evaluator(name="m", user_prompt="prompt", is_global=True) assert exc_info.value is original + assert exc_info.value.__cause__ is None def test_422_is_raised_as_value_error(self, mock_create_scorer, mock_create_version) -> None: # Given: the scorer post answers with a validation error From f310b3790cc693523f565e808029bf6dbf287838 Mon Sep 17 00:00:00 2001 From: quinn-galileo Date: Mon, 19 Oct 2026 12:44:16 -0400 Subject: [PATCH 4/9] fix: give a refused decision version its own permission hint Co-Authored-By: Claude Opus 5.5 (1M context) --- README.md | 3 ++- src/splunk_ao/evaluators.py | 14 +++++++++----- tests/test_evaluator_permissions.py | 7 +++++-- 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 8eb930b6..88d95a8c 100644 --- a/README.md +++ b/README.md @@ -631,7 +631,8 @@ run_experiment( New evaluators are scoped to the current project (`SPLUNK_AO_PROJECT_ID` or `SPLUNK_AO_PROJECT`) by default. Pass `project_ids` to choose other projects, or `is_global=True` for a global evaluator. Only org admins can create global -evaluators, and changing or deleting an evaluator needs the owner or editor role on one of its projects. +evaluators, and updating or deleting an evaluator needs the owner or editor role on one of its projects. Changing +scope needs the owner or editor role on each project added or removed, and org admin to make it global. ```python from splunk_ao import EvaluatorPermissionError, LlmEvaluator diff --git a/src/splunk_ao/evaluators.py b/src/splunk_ao/evaluators.py index a92ee60b..18326911 100644 --- a/src/splunk_ao/evaluators.py +++ b/src/splunk_ao/evaluators.py @@ -103,6 +103,10 @@ def __init__(self, scorer_id: str, cause: Exception) -> None: "Creating a project-scoped evaluator needs the owner or editor role on every project in project_ids " "(or on the current project when it is the default). Ask a project owner for access, or choose other projects." ) +_DECISION_VERSION_DENIED_HINT = ( + "The server refused the decision version request, so the new evaluator was deleted. " + "Check that decision evaluators are enabled for your organization." +) _SCOPE_CHANGE_DENIED_HINT = ( "Changing an evaluator's scope needs the owner or editor role on each project added or removed; " "making it global needs the org admin role." @@ -479,8 +483,8 @@ def create_decision_evaluator( If an argument is invalid or the API answers with a validation error. If the scorer was created but its version was rejected, the scorer is deleted before the error is raised. SplunkAOAPIError - For a definitive rejection (400, 403, 404, 422), for example 403 when the feature is not enabled. - The scorer is deleted before the error is raised. + For a definitive rejection of the version request (400, 404, 422). The scorer is deleted before the + error is raised. A 403 on either request is raised as ``EvaluatorPermissionError``. DecisionVersionOutcomeUnknownError For a timeout, network error, 5xx or any other failure. The version may have been saved, so the scorer is kept and the error names its id. @@ -524,7 +528,7 @@ def create_decision_evaluator( except Exception as error: if self._was_rejected(error): self._delete_scorer_quietly(scorer.id) - with _translate_forbidden(create_hint): + with _translate_forbidden(_DECISION_VERSION_DENIED_HINT): raise _logger.warning( "Decision evaluator version request failed with an unknown outcome; scorer_id=%s was kept: %s", @@ -751,8 +755,8 @@ def create_decision_evaluator( If an argument is invalid or the API answers with a validation error. If the scorer was created but its version was rejected, the scorer is deleted before the error is raised. SplunkAOAPIError - For a definitive rejection (400, 403, 404, 422), for example 403 when the feature is not enabled. - The scorer is deleted before the error is raised. + For a definitive rejection of the version request (400, 404, 422). The scorer is deleted before the + error is raised. A 403 on either request is raised as ``EvaluatorPermissionError``. DecisionVersionOutcomeUnknownError For a timeout, network error, 5xx or any other failure. The version may have been saved, so the scorer is kept and the error names its id. diff --git a/tests/test_evaluator_permissions.py b/tests/test_evaluator_permissions.py index 55786646..f7cba57b 100644 --- a/tests/test_evaluator_permissions.py +++ b/tests/test_evaluator_permissions.py @@ -210,10 +210,13 @@ def test_version_post_403_is_translated_and_the_scorer_is_still_deleted( mock_create_scorer.sync.return_value = scorer mock_create_version.sync.side_effect = _forbidden() - # When/Then: the permission error carries the scoped hint and the orphan scorer is deleted + # When/Then: the permission error explains the refused version (not a create-scope problem) and the + # orphan scorer is deleted with pytest.raises(EvaluatorPermissionError) as exc_info: create_decision_evaluator(name="tone", question="q", project_ids=["p1"]) - _assert_permission_error(exc_info.value, SCOPED_HINT) + _assert_permission_error(exc_info.value, "decision version request") + assert SCOPED_HINT not in str(exc_info.value) + assert GLOBAL_HINT not in str(exc_info.value) mock_delete.sync.assert_called_once() assert mock_delete.sync.call_args.kwargs["scorer_id"] == scorer.id From b1940bee10a92598647e96eb494338453e2d43fd Mon Sep 17 00:00:00 2001 From: quinn-galileo Date: Mon, 19 Oct 2026 12:46:51 -0400 Subject: [PATCH 5/9] fix: do not claim the refused decision evaluator was deleted Co-Authored-By: Claude Opus 5.5 (1M context) --- src/splunk_ao/evaluators.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/splunk_ao/evaluators.py b/src/splunk_ao/evaluators.py index 18326911..1b9e7f2d 100644 --- a/src/splunk_ao/evaluators.py +++ b/src/splunk_ao/evaluators.py @@ -104,7 +104,7 @@ def __init__(self, scorer_id: str, cause: Exception) -> None: "(or on the current project when it is the default). Ask a project owner for access, or choose other projects." ) _DECISION_VERSION_DENIED_HINT = ( - "The server refused the decision version request, so the new evaluator was deleted. " + "The server refused the decision version request, and the SDK tried to delete the new evaluator. " "Check that decision evaluators are enabled for your organization." ) _SCOPE_CHANGE_DENIED_HINT = ( From e233c2b202c93a73d3b28b672a42e168cfc4ab6e Mon Sep 17 00:00:00 2001 From: quinn-galileo Date: Mon, 19 Oct 2026 12:54:11 -0400 Subject: [PATCH 6/9] fix: sharpen permission hints and tidy tests, README and changelog Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 9 +- README.md | 15 +- src/splunk_ao/evaluator.py | 43 +++--- src/splunk_ao/evaluators.py | 31 ++-- tests/test_evaluator_permissions.py | 210 ++++++++++++++++++++-------- 5 files changed, 210 insertions(+), 98 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06735208..34535bb2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,9 +24,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `Evaluator.is_global` and `Evaluator.scope_projects` report the scope the API returned, after `create()`, `get()`, `list()` and `refresh()`. - `EvaluatorPermissionError`, a `ForbiddenError` subclass, replaces the generic 403 message when creating, - updating, deleting or re-scoping an evaluator is refused. It says what is needed: org admin to create a global - evaluator (or pass `project_ids`), the owner or editor role on the projects for scoped create, update, delete and - scope changes. The server response body is kept in the message. + updating, deleting or re-scoping an evaluator is refused, and says what is needed. Create needs the owner or + editor role on every listed project. Update and delete need it on one of the evaluator's projects. A scope change + needs it on one of the current projects and on each project added or removed. Anything on a global evaluator, + including creating one, needs the org admin role. The server response body is kept in the message. - `Evaluator.set_scope(project_ids=[...])` and `set_scope(is_global=True)` replace an evaluator's scope through `PUT /scorers/{scorer_id}/scope`. Promoting to global is org admin only. @@ -46,7 +47,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `Evaluators.delete_evaluator()` and `Evaluator.refresh()` now say "not found, or not shared with your projects" when the evaluator is missing, since the server hides evaluators outside the caller's projects. `Evaluator.get()` - still returns `None` in that case. A 404 from update, delete or `set_scope` stays a `NotFoundError`. + still returns `None` in that case. - Creating an LLM or code evaluator no longer fails with an `AttributeError` when the scorer request is answered with a validation error; the detail is raised as a `ValueError`, as it already was for decision evaluators. - `SplunkAOTracingProcessor` (OpenAI Agents) now records Responses API LLM spans, the Agents SDK diff --git a/README.md b/README.md index 88d95a8c..ceb55a5d 100644 --- a/README.md +++ b/README.md @@ -629,10 +629,12 @@ run_experiment( #### Evaluators -New evaluators are scoped to the current project (`SPLUNK_AO_PROJECT_ID` or `SPLUNK_AO_PROJECT`) by default. Pass -`project_ids` to choose other projects, or `is_global=True` for a global evaluator. Only org admins can create global -evaluators, and updating or deleting an evaluator needs the owner or editor role on one of its projects. Changing -scope needs the owner or editor role on each project added or removed, and org admin to make it global. +New evaluators are scoped to the current project (decorator context, then `SPLUNK_AO_PROJECT_ID`, then +`SPLUNK_AO_PROJECT`) by default. Pass `project_ids` to choose other projects, or `is_global=True` for a global +evaluator. With no current project and no `project_ids`, the evaluator is created global, which only org admins can do. +Updating or deleting an evaluator needs the owner or editor role on one of its projects (org admin for a global +evaluator). Changing scope needs the owner or editor role on one of its current projects and on each project added or +removed; making it global, or changing the scope of a global evaluator, needs the org admin role. ```python from splunk_ao import EvaluatorPermissionError, LlmEvaluator @@ -647,8 +649,9 @@ except EvaluatorPermissionError as error: print(error) # says which role is missing and how to fix it ``` -`EvaluatorPermissionError` subclasses `ForbiddenError`. Looking up an evaluator by name returns `None` when it does -not exist or is not shared with your projects. +`EvaluatorPermissionError` subclasses `ForbiddenError`. `Evaluator.get(name=...)` returns `None` when the evaluator +does not exist or is not shared with your projects. The server answers 404 rather than 403 for an evaluator outside +your projects on some paths, so `update`, `delete` and `set_scope` can raise `NotFoundError` in that case. ### Sessions diff --git a/src/splunk_ao/evaluator.py b/src/splunk_ao/evaluator.py index 2da16b2c..82b33780 100644 --- a/src/splunk_ao/evaluator.py +++ b/src/splunk_ao/evaluator.py @@ -556,7 +556,7 @@ def update(self, **kwargs: Any) -> Evaluator: logger.info(f"Evaluator.update: id='{self.id}' name='{self.name}' - started") try: config = SplunkAOConfig.get() - with _translate_forbidden(_edit_denied_hint("update")): + with _translate_forbidden(_edit_denied_hint("Updating")): response = update_scorers_scorer_id_patch.sync(scorer_id=self.id, client=config.api_client, body=body) except Exception as e: self._set_state(SyncState.FAILED_SYNC, error=e) @@ -575,56 +575,57 @@ def update(self, **kwargs: Any) -> Evaluator: def set_scope(self, *, project_ids: builtins.list[str] | None = None, is_global: bool = False) -> Evaluator: """ - Replace this metric's scope on the API. + Replace this evaluator's scope on the API. - This is a full replace: the metric ends up scoped to exactly ``project_ids``, or global. It never + This is a full replace: the evaluator ends up scoped to exactly ``project_ids``, or global. It never falls back to the current project. On success the instance is updated with the API response and returned in SYNCED state. Parameters ---------- project_ids : list[str], optional - Projects to scope the metric to. Required (and non-empty) unless ``is_global=True``. + Projects to scope the evaluator to. Required (and non-empty) unless ``is_global=True``. is_global : bool - Make the metric global. Cannot be combined with ``project_ids``. Org admins only. + Make the evaluator global. Cannot be combined with ``project_ids``. Org admins only. Returns ------- - Evaluator: This metric instance with updated attributes from the API. + Evaluator: This evaluator instance with updated attributes from the API. Raises ------ - ValidationError: If this is a local metric, or the arguments are invalid (``is_global=True`` with + ValidationError: If this is a local evaluator, or the arguments are invalid (``is_global=True`` with ``project_ids``, ``project_ids`` missing, empty or a single string). - ValueError: If the metric ID is not set, the metric is deleted, or the metric is in FAILED_SYNC state. + ValueError: If the evaluator ID is not set, the evaluator is deleted, or the evaluator is in FAILED_SYNC state. APIError: If the API returns a validation error or empty response. - EvaluatorPermissionError: If the API answers 403: the owner or editor role is needed on each project - added or removed, and the org admin role to make the metric global. - NotFoundError: If the API answers 404. The server also answers 404 for a metric that exists but - isn't shared with your projects. + EvaluatorPermissionError: If the API answers 403: changing an evaluator's scope needs the owner or + editor role on one of its current projects and on each project added or removed. Making it global, + or changing the scope of a global evaluator, needs the org admin role. + NotFoundError: If the API answers 404. The server also answers 404 for an evaluator that exists but + isn't shared with your projects, or the server is too old to support evaluator scopes. Exception: If the API call fails (state is set to FAILED_SYNC). Examples -------- - metric = Evaluator.get(name="factuality-checker") - metric.set_scope(project_ids=[""]) - metric.set_scope(is_global=True) # org admins only + evaluator = Evaluator.get(name="factuality-checker") + evaluator.set_scope(project_ids=[""]) + evaluator.set_scope(is_global=True) # org admins only """ if isinstance(self, LocalEvaluator): - raise ValidationError("Local metrics don't exist on the server and can't change scope.") + raise ValidationError("Local evaluators don't exist on the server and can't change scope.") if self.id is None: - raise ValueError("Evaluator ID is not set. Cannot change the scope of a local-only metric.") + raise ValueError("Evaluator ID is not set. Cannot change the scope of a local-only evaluator.") if self.sync_state == SyncState.DELETED: - raise ValueError("Cannot change the scope of a deleted metric.") + raise ValueError("Cannot change the scope of a deleted evaluator.") if self.sync_state == SyncState.FAILED_SYNC: raise ValueError( - "Cannot change the scope of a metric in FAILED_SYNC state. " + "Cannot change the scope of an evaluator in FAILED_SYNC state. " "Call refresh() to re-sync from the API, then retry your changes." ) if is_global: if project_ids: - raise ValidationError("A metric cannot be both global and scoped: pass is_global=True or project_ids.") + raise ValidationError("An evaluator cannot be both global and scoped: pass is_global=True or project_ids.") body = UpdateScorerScopeRequest(is_global=True) else: # Typed as object so the str guard is not flagged unreachable for a list[str] parameter. @@ -632,7 +633,7 @@ def set_scope(self, *, project_ids: builtins.list[str] | None = None, is_global: if isinstance(requested, str): raise ValidationError("project_ids must be a list of project ids, not a single string.") if not project_ids: - raise ValidationError("project_ids must not be empty. Pass is_global=True to make the metric global.") + raise ValidationError("project_ids must not be empty. Pass is_global=True to make the evaluator global.") body = UpdateScorerScopeRequest(is_global=False, project_ids=list(dict.fromkeys(project_ids))) logger.info(f"Evaluator.set_scope: id='{self.id}' name='{self.name}' - started") diff --git a/src/splunk_ao/evaluators.py b/src/splunk_ao/evaluators.py index 1b9e7f2d..d8e4b71c 100644 --- a/src/splunk_ao/evaluators.py +++ b/src/splunk_ao/evaluators.py @@ -103,27 +103,38 @@ def __init__(self, scorer_id: str, cause: Exception) -> None: "Creating a project-scoped evaluator needs the owner or editor role on every project in project_ids " "(or on the current project when it is the default). Ask a project owner for access, or choose other projects." ) +_EXPLICIT_GLOBAL_CREATE_DENIED_HINT = ( + "Only org admins can create global evaluators. Drop is_global=True and pass project_ids=[...] " + "to scope the evaluator to your projects." +) +_DECISION_ENABLED_HINT = " Decision evaluators may also not be enabled for your organization." _DECISION_VERSION_DENIED_HINT = ( - "The server refused the decision version request, and the SDK tried to delete the new evaluator. " + "The server refused to save the decision evaluator's configuration, so the SDK deleted the new evaluator " + "(a warning with its id is logged if that cleanup failed). " "Check that decision evaluators are enabled for your organization." ) _SCOPE_CHANGE_DENIED_HINT = ( - "Changing an evaluator's scope needs the owner or editor role on each project added or removed; " - "making it global needs the org admin role." + "Changing an evaluator's scope needs the owner or editor role on one of its current projects and on each " + "project added or removed. Making it global, or changing the scope of a global evaluator, needs the org admin " + "role." ) -def _edit_denied_hint(action: str) -> str: - """Hint for a 403 when updating or deleting an evaluator.""" +def _edit_denied_hint(gerund: str) -> str: + """Hint for a 403 when updating or deleting an evaluator; ``gerund`` is "Updating" or "Deleting".""" return ( - f"You can't {action} this evaluator. It needs the owner or editor role on one of the evaluator's projects " + f"{gerund} an evaluator needs the owner or editor role on one of its projects " "(org admin for a global evaluator)." ) def _create_denied_hint(scope_is_global: bool | Unset) -> str: """Hint for a 403 on create, chosen from the scope that was actually sent.""" - return _SCOPED_CREATE_DENIED_HINT if scope_is_global is False else _GLOBAL_CREATE_DENIED_HINT + if scope_is_global is False: + return _SCOPED_CREATE_DENIED_HINT + if scope_is_global is True: + return _EXPLICIT_GLOBAL_CREATE_DENIED_HINT + return _GLOBAL_CREATE_DENIED_HINT @contextmanager @@ -307,7 +318,7 @@ def delete_evaluator(self, name: str) -> None: raise ValueError(f"Evaluator with name '{name}' not found, or not shared with your projects.") for scorer in scorers_to_delete: - with _translate_forbidden(_edit_denied_hint("delete")): + with _translate_forbidden(_edit_denied_hint("Deleting")): response = delete_scorer_scorers_scorer_id_delete.sync( scorer_id=scorer.id, client=self.config.api_client ) @@ -509,8 +520,7 @@ def create_decision_evaluator( project_ids=scope_project_ids, ) - create_hint = _create_denied_hint(scope_is_global) - with _translate_forbidden(create_hint): + with _translate_forbidden(_create_denied_hint(scope_is_global) + _DECISION_ENABLED_HINT): scorer = create_scorers_post.sync(body=create_scorer_request, client=self.config.api_client) if isinstance(scorer, HTTPValidationError): raise ValueError(scorer.detail) @@ -528,6 +538,7 @@ def create_decision_evaluator( except Exception as error: if self._was_rejected(error): self._delete_scorer_quietly(scorer.id) + # The bare raise re-raises the 403 inside the with, which translates it. with _translate_forbidden(_DECISION_VERSION_DENIED_HINT): raise _logger.warning( diff --git a/tests/test_evaluator_permissions.py b/tests/test_evaluator_permissions.py index f7cba57b..0b0cc433 100644 --- a/tests/test_evaluator_permissions.py +++ b/tests/test_evaluator_permissions.py @@ -25,10 +25,23 @@ pytestmark = pytest.mark.usefixtures("no_current_project") SERVER_BODY = b'{"detail": "denied by server"}' -GLOBAL_HINT = "Only org admins can create global evaluators" +UNSET_GLOBAL_HINT = ( + "Only org admins can create global evaluators. Pass project_ids=[...] to scope the evaluator to your projects, " + "or set SPLUNK_AO_PROJECT / SPLUNK_AO_PROJECT_ID so it defaults to one." +) +EXPLICIT_GLOBAL_HINT = "Only org admins can create global evaluators. Drop is_global=True and pass project_ids=[...]" SCOPED_HINT = "owner or editor role on every project in project_ids" -EDIT_HINT = "owner or editor role on one of the evaluator's projects" -SCOPE_HINT = "owner or editor role on each project added or removed" +DECISION_ENABLED_HINT = "Decision evaluators may also not be enabled for your organization." +UPDATE_HINT = "Updating an evaluator needs the owner or editor role on one of its projects (org admin for a global" +DELETE_HINT = "Deleting an evaluator needs the owner or editor role on one of its projects (org admin for a global" +SCOPE_HINT = ( + "owner or editor role on one of its current projects and on each project added or removed. " + "Making it global, or changing the scope of a global evaluator, needs the org admin role." +) +GLOBAL_CASES = [({}, UNSET_GLOBAL_HINT), ({"is_global": True}, EXPLICIT_GLOBAL_HINT)] +GLOBAL_IDS = ["unset_scope", "explicit_global"] +ALL_CREATE_CASES = [*GLOBAL_CASES, ({"project_ids": ["p1"]}, SCOPED_HINT)] +ALL_CREATE_IDS = [*GLOBAL_IDS, "scoped"] def _forbidden() -> ForbiddenError: @@ -65,10 +78,11 @@ def _synced(evaluator: Evaluator) -> Evaluator: class TestEvaluatorPermissionErrorClass: def test_is_forbidden_error_and_exported(self) -> None: - # Given/When: the error built the way the SDK builds it + # Given: the error built the way the SDK builds it + # When: it is created and formatted error = EvaluatorPermissionError(403, SERVER_BODY, "Only org admins can do this.") - # Then: it is a ForbiddenError with the hint first and the server body after it + # Then: it is a ForbiddenError with the hint first and the server body after it, and it is exported assert isinstance(error, ForbiddenError) assert error.status_code == 403 assert str(error).startswith("Only org admins can do this. (HTTP 403)") @@ -82,22 +96,20 @@ def test_is_forbidden_error_and_exported(self) -> None: @patch("splunk_ao.evaluators.create_scorers_post") class TestCreateLlmDenied: @pytest.mark.parametrize("use_function", [False, True], ids=["method", "module_function"]) - @pytest.mark.parametrize("kwargs", [{}, {"is_global": True}], ids=["unset_scope", "explicit_global"]) + @pytest.mark.parametrize(("kwargs", "hint"), GLOBAL_CASES, ids=GLOBAL_IDS) def test_global_create_403_names_admins_and_the_fix( - self, mock_create_scorer, mock_create_version, use_function, kwargs + self, mock_create_scorer, mock_create_version, use_function, kwargs, hint ) -> None: - # Given: the server refuses the create + # Given: the server refuses a create whose scope is unset or global mock_create_scorer.sync.side_effect = _forbidden() + create = create_custom_llm_evaluator if use_function else Evaluators().create_custom_llm_evaluator - # When/Then: the error says only org admins can create global evaluators and how to scope + # When: the evaluator is created with pytest.raises(EvaluatorPermissionError) as exc_info: - if use_function: - create_custom_llm_evaluator("m", "prompt", **kwargs) - else: - Evaluators().create_custom_llm_evaluator(name="m", user_prompt="prompt", **kwargs) - _assert_permission_error(exc_info.value, GLOBAL_HINT) - assert "project_ids=[...]" in str(exc_info.value) - assert "SPLUNK_AO_PROJECT / SPLUNK_AO_PROJECT_ID" in str(exc_info.value) + create(name="m", user_prompt="prompt", **kwargs) + + # Then: the error says only org admins can create global evaluators and how to scope, with no version request + _assert_permission_error(exc_info.value, hint) mock_create_version.sync.assert_not_called() @pytest.mark.parametrize("use_function", [False, True], ids=["method", "module_function"]) @@ -106,15 +118,15 @@ def test_scoped_create_403_names_the_project_role( ) -> None: # Given: a scoped create the server refuses mock_create_scorer.sync.side_effect = _forbidden() + create = create_custom_llm_evaluator if use_function else Evaluators().create_custom_llm_evaluator - # When/Then: the message is about the project role, not about admins + # When: the evaluator is created with pytest.raises(EvaluatorPermissionError) as exc_info: - if use_function: - create_custom_llm_evaluator("m", "prompt", project_ids=["p1"]) - else: - Evaluators().create_custom_llm_evaluator(name="m", user_prompt="prompt", project_ids=["p1"]) + create(name="m", user_prompt="prompt", project_ids=["p1"]) + + # Then: the message is about the project role, not about admins _assert_permission_error(exc_info.value, SCOPED_HINT) - assert GLOBAL_HINT not in str(exc_info.value) + assert "org admins" not in str(exc_info.value) def test_hint_follows_the_resolved_scope_not_the_instance( self, mock_create_scorer, mock_create_version, monkeypatch @@ -123,10 +135,12 @@ def test_hint_follows_the_resolved_scope_not_the_instance( monkeypatch.setenv("SPLUNK_AO_PROJECT_ID", "proj-1") mock_create_scorer.sync.side_effect = _forbidden() - # When/Then: the scoped hint is used because the resolved scope is not global + # When: the evaluator is created with patch("splunk_ao.evaluators._resolve_project", return_value=MagicMock(id="proj-1")): with pytest.raises(EvaluatorPermissionError) as exc_info: Evaluators().create_custom_llm_evaluator(name="m", user_prompt="prompt") + + # Then: the scoped hint is used because the resolved scope is not global _assert_permission_error(exc_info.value, SCOPED_HINT) def test_already_translated_error_is_not_wrapped_again(self, mock_create_scorer, mock_create_version) -> None: @@ -134,9 +148,11 @@ def test_already_translated_error_is_not_wrapped_again(self, mock_create_scorer, original = EvaluatorPermissionError(403, b"x", "Custom hint.") mock_create_scorer.sync.side_effect = original - # When/Then: the same object comes out + # When: the evaluator is created with pytest.raises(EvaluatorPermissionError) as exc_info: Evaluators().create_custom_llm_evaluator(name="m", user_prompt="prompt", is_global=True) + + # Then: the same object comes out, with no cause chained assert exc_info.value is original assert exc_info.value.__cause__ is None @@ -144,13 +160,16 @@ def test_422_is_raised_as_value_error(self, mock_create_scorer, mock_create_vers # Given: the scorer post answers with a validation error mock_create_scorer.sync.return_value = HTTPValidationError(detail="bad name") - # When/Then: the detail surfaces and no version request is made + # When/Then: the detail surfaces as ValueError and no version request is made with pytest.raises(ValueError, match="bad name"): Evaluators().create_custom_llm_evaluator(name="m", user_prompt="prompt", is_global=True) mock_create_version.sync.assert_not_called() def test_empty_response_is_raised_as_value_error(self, mock_create_scorer, mock_create_version) -> None: + # Given: the scorer post returns nothing mock_create_scorer.sync.return_value = None + + # When/Then: a ValueError is raised instead of reading .id off None with pytest.raises(ValueError, match="Failed to create custom LLM evaluator"): Evaluators().create_custom_llm_evaluator(name="m", user_prompt="prompt", is_global=True) @@ -161,20 +180,26 @@ def test_llm_evaluator_create_keeps_the_error_and_marks_failed( mock_create_scorer.sync.side_effect = _forbidden() evaluator = LlmEvaluator(name="m", prompt="p") - # When/Then: the permission error is raised once, and the state is FAILED_SYNC + # When: it is created with pytest.raises(EvaluatorPermissionError) as exc_info: evaluator.create() - _assert_permission_error(exc_info.value, GLOBAL_HINT) - assert exc_info.value.__cause__ is not None + + # Then: the permission error is raised once (its cause is the plain 403) and the state is FAILED_SYNC + _assert_permission_error(exc_info.value, UNSET_GLOBAL_HINT) assert type(exc_info.value.__cause__) is ForbiddenError assert evaluator.sync_state == SyncState.FAILED_SYNC def test_llm_evaluator_create_with_projects_says_owner_or_editor( self, mock_create_scorer, mock_create_version ) -> None: + # Given: an LlmEvaluator scoped to a project the server refuses mock_create_scorer.sync.side_effect = _forbidden() + + # When: it is created with pytest.raises(EvaluatorPermissionError) as exc_info: LlmEvaluator(name="m", prompt="p", project_ids=["p1"]).create() + + # Then: the hint names the project role _assert_permission_error(exc_info.value, SCOPED_HINT) @@ -183,11 +208,7 @@ def test_llm_evaluator_create_with_projects_says_owner_or_editor( @patch("splunk_ao.evaluators.create_scorers_post") class TestCreateDecisionDenied: @pytest.mark.parametrize("use_function", [False, True], ids=["method", "module_function"]) - @pytest.mark.parametrize( - ("kwargs", "hint"), - [({}, GLOBAL_HINT), ({"is_global": True}, GLOBAL_HINT), ({"project_ids": ["p1"]}, SCOPED_HINT)], - ids=["unset_scope", "explicit_global", "scoped"], - ) + @pytest.mark.parametrize(("kwargs", "hint"), ALL_CREATE_CASES, ids=ALL_CREATE_IDS) def test_scorer_post_403_is_translated( self, mock_create_scorer, mock_create_version, mock_delete, use_function, kwargs, hint ) -> None: @@ -195,10 +216,13 @@ def test_scorer_post_403_is_translated( mock_create_scorer.sync.side_effect = _forbidden() create = create_decision_evaluator if use_function else Evaluators().create_decision_evaluator - # When/Then: the hint matches the resolved scope; nothing else was called + # When: the decision evaluator is created with pytest.raises(EvaluatorPermissionError) as exc_info: create(name="tone", question="q", **kwargs) + + # Then: the hint matches the resolved scope, mentions the feature gate, and nothing else was called _assert_permission_error(exc_info.value, hint) + assert DECISION_ENABLED_HINT in exc_info.value.message mock_create_version.sync.assert_not_called() mock_delete.sync.assert_not_called() @@ -210,24 +234,30 @@ def test_version_post_403_is_translated_and_the_scorer_is_still_deleted( mock_create_scorer.sync.return_value = scorer mock_create_version.sync.side_effect = _forbidden() - # When/Then: the permission error explains the refused version (not a create-scope problem) and the - # orphan scorer is deleted + # When: the decision evaluator is created with pytest.raises(EvaluatorPermissionError) as exc_info: create_decision_evaluator(name="tone", question="q", project_ids=["p1"]) - _assert_permission_error(exc_info.value, "decision version request") + + # Then: the error explains the refused version (not a create-scope problem) and the orphan scorer is deleted + _assert_permission_error(exc_info.value, "refused to save the decision evaluator's configuration") assert SCOPED_HINT not in str(exc_info.value) - assert GLOBAL_HINT not in str(exc_info.value) + assert "org admins" not in str(exc_info.value) mock_delete.sync.assert_called_once() assert mock_delete.sync.call_args.kwargs["scorer_id"] == scorer.id def test_decision_evaluator_create_raises_one_permission_error( self, mock_create_scorer, mock_create_version, mock_delete ) -> None: + # Given: a DecisionEvaluator with the default scope the server refuses mock_create_scorer.sync.side_effect = _forbidden() evaluator = DecisionEvaluator(name="tone", question="q") + + # When: it is created with pytest.raises(EvaluatorPermissionError) as exc_info: evaluator.create() - _assert_permission_error(exc_info.value, GLOBAL_HINT) + + # Then: one permission error (cause is the plain 403) and FAILED_SYNC + _assert_permission_error(exc_info.value, UNSET_GLOBAL_HINT) assert type(exc_info.value.__cause__) is ForbiddenError assert evaluator.sync_state == SyncState.FAILED_SYNC @@ -235,20 +265,18 @@ def test_decision_evaluator_create_raises_one_permission_error( @patch("splunk_ao.evaluator.SplunkAOConfig.get") @patch("splunk_ao.evaluator.create_scorers_post") class TestCreateCodeDenied: - @pytest.mark.parametrize( - ("kwargs", "hint"), - [({}, GLOBAL_HINT), ({"is_global": True}, GLOBAL_HINT), ({"project_ids": ["p1"]}, SCOPED_HINT)], - ids=["unset_scope", "explicit_global", "scoped"], - ) + @pytest.mark.parametrize(("kwargs", "hint"), ALL_CREATE_CASES, ids=ALL_CREATE_IDS) def test_403_is_translated_by_resolved_scope(self, mock_create_scorers, mock_config, kwargs, hint) -> None: # Given: code validation passes and the scorer post is refused mock_create_scorers.sync.side_effect = _forbidden() evaluator = CodeEvaluator(name="c", code="def f(): pass", **kwargs) - # When/Then + # When: the code evaluator is created with patch.object(CodeEvaluator, "_validate_code", return_value="{}"): with pytest.raises(EvaluatorPermissionError) as exc_info: evaluator.create() + + # Then: the hint matches the resolved scope and the state is FAILED_SYNC _assert_permission_error(exc_info.value, hint) assert type(exc_info.value.__cause__) is ForbiddenError assert evaluator.sync_state == SyncState.FAILED_SYNC @@ -271,78 +299,112 @@ def test_update_403_is_translated_and_marks_failed(self, mock_config, mock_patch evaluator = _synced(LlmEvaluator(name="m", prompt="p")) mock_patch.sync.side_effect = _forbidden() - # When/Then + # When: it is updated with pytest.raises(EvaluatorPermissionError) as exc_info: evaluator.update(name="new") - _assert_permission_error(exc_info.value, EDIT_HINT) - assert "org admin for a global evaluator" in str(exc_info.value) + + # Then: the edit hint is raised and the state is FAILED_SYNC + _assert_permission_error(exc_info.value, UPDATE_HINT) assert evaluator.sync_state == SyncState.FAILED_SYNC @patch("splunk_ao.evaluator.update_scorers_scorer_id_patch") @patch("splunk_ao.evaluator.SplunkAOConfig.get") def test_update_404_stays_not_found(self, mock_config, mock_patch) -> None: + # Given: the server answers 404 (it hides evaluators outside the caller's projects) evaluator = _synced(LlmEvaluator(name="m", prompt="p")) mock_patch.sync.side_effect = NotFoundError(404, b"gone") + + # When: the evaluator is updated with pytest.raises(NotFoundError) as exc_info: evaluator.update(name="new") + + # Then: it stays a NotFoundError, never a permission error, and the state is FAILED_SYNC as before assert not isinstance(exc_info.value, EvaluatorPermissionError) assert evaluator.sync_state == SyncState.FAILED_SYNC @patch("splunk_ao.evaluators.delete_scorer_scorers_scorer_id_delete") @patch("splunk_ao.scorers.Scorers.list") def test_delete_evaluator_403_is_translated(self, mock_list, mock_delete) -> None: + # Given: a listed scorer whose delete the server refuses mock_list.return_value = [_scorer_response(str(uuid4()))] mock_delete.sync.side_effect = _forbidden() + + # When: it is deleted by name through the service with pytest.raises(EvaluatorPermissionError) as exc_info: Evaluators().delete_evaluator(name="ev") - _assert_permission_error(exc_info.value, EDIT_HINT) + + # Then: the delete hint is raised + _assert_permission_error(exc_info.value, DELETE_HINT) @patch("splunk_ao.evaluators.delete_scorer_scorers_scorer_id_delete") @patch("splunk_ao.scorers.Scorers.list") def test_delete_by_name_403_is_translated(self, mock_list, mock_delete) -> None: + # Given: a listed scorer whose delete the server refuses mock_list.return_value = [_scorer_response(str(uuid4()))] mock_delete.sync.side_effect = _forbidden() + + # When: Evaluator.delete_by_name is called with pytest.raises(EvaluatorPermissionError) as exc_info: Evaluator.delete_by_name("ev") - _assert_permission_error(exc_info.value, EDIT_HINT) + + # Then: the delete hint is raised + _assert_permission_error(exc_info.value, DELETE_HINT) @patch("splunk_ao.evaluators.delete_scorer_scorers_scorer_id_delete") @patch("splunk_ao.scorers.Scorers.list") def test_instance_delete_403_is_translated_and_marks_failed(self, mock_list, mock_delete) -> None: + # Given: a synced evaluator whose delete the server refuses evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) mock_list.return_value = [_scorer_response(str(uuid4()))] mock_delete.sync.side_effect = _forbidden() + + # When: it is deleted with pytest.raises(EvaluatorPermissionError) as exc_info: evaluator.delete() - _assert_permission_error(exc_info.value, EDIT_HINT) + + # Then: the delete hint is raised and the state is FAILED_SYNC + _assert_permission_error(exc_info.value, DELETE_HINT) assert evaluator.sync_state == SyncState.FAILED_SYNC @patch("splunk_ao.evaluators.delete_scorer_scorers_scorer_id_delete") @patch("splunk_ao.scorers.Scorers.list") def test_delete_404_stays_not_found(self, mock_list, mock_delete) -> None: + # Given: the server answers 404 to the delete mock_list.return_value = [_scorer_response(str(uuid4()))] mock_delete.sync.side_effect = NotFoundError(404, b"gone") + + # When: the evaluator is deleted with pytest.raises(NotFoundError) as exc_info: Evaluators().delete_evaluator(name="ev") + + # Then: it stays a NotFoundError, never a permission error assert not isinstance(exc_info.value, EvaluatorPermissionError) class TestNotFoundWording: @patch("splunk_ao.scorers.Scorers.list", return_value=[]) def test_delete_evaluator_name_not_in_list(self, mock_list) -> None: + # Given: no scorer with that name is visible + # When/Then: the error says it may simply not be shared with the caller's projects with pytest.raises(ValueError, match="not found, or not shared with your projects"): Evaluators().delete_evaluator(name="ghost") @patch("splunk_ao.evaluator.Scorers") def test_refresh_id_not_in_list(self, mock_scorers) -> None: + # Given: a synced evaluator whose id is no longer in the visible list evaluator = _synced(LlmEvaluator(name="m", prompt="p")) mock_scorers.return_value.list.return_value = [] + + # When/Then: refresh says it may not be shared with the caller's projects with pytest.raises(ValueError, match="not found, or not shared with your projects"): evaluator.refresh() @patch("splunk_ao.evaluator.Scorers") def test_get_still_returns_none_and_documents_why(self, mock_scorers) -> None: + # Given: no visible scorers mock_scorers.return_value.list.return_value = [] + + # When/Then: get returns None by name and id, and its docstring explains the sharing case assert Evaluator.get(name="ghost") is None assert Evaluator.get(id="ghost-id") is None assert "isn't shared with the caller's projects" in (Evaluator.get.__doc__ or "") @@ -358,10 +420,10 @@ def test_scoped_request_populates_instance_and_returns_self(self, mock_config, m evaluator.id, is_global=False, scope_projects=[{"id": "a", "name": "A"}, {"id": "b", "name": "B"}] ) - # When + # When: its scope is replaced, with a duplicate id in the request result = evaluator.set_scope(project_ids=["a", "b", "a"]) - # Then: the full-replace request is deduplicated, the response is applied, the create-time scope is untouched + # Then: the request is deduplicated, the response is applied, and the create-time scope is untouched assert result is evaluator kwargs = mock_put.sync.call_args.kwargs assert kwargs["scorer_id"] == evaluator.id @@ -378,11 +440,14 @@ def test_scoped_request_populates_instance_and_returns_self(self, mock_config, m assert evaluator._create_is_global is None def test_global_request_sends_no_project_ids(self, mock_config, mock_put) -> None: + # Given: a synced evaluator and a server that reports it global evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) mock_put.sync.return_value = _scorer_response(evaluator.id, is_global=True, scope_projects=[]) + # When: it is made global evaluator.set_scope(is_global=True) + # Then: the request carries is_global=True and no project_ids, and the response is applied body = mock_put.sync.call_args.kwargs["body"] assert body.is_global is True assert body.project_ids is UNSET @@ -402,56 +467,87 @@ def test_global_request_sends_no_project_ids(self, mock_config, mock_put) -> Non ], ) def test_invalid_arguments_make_no_request_and_keep_state(self, mock_config, mock_put, kwargs, message) -> None: + # Given: a synced evaluator evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) + + # When: set_scope gets conflicting, missing, empty or bare-string project ids with pytest.raises(ValidationError, match=message): evaluator.set_scope(**kwargs) + + # Then: no request was made and the state is unchanged mock_put.sync.assert_not_called() assert evaluator.is_synced() def test_local_evaluator_is_rejected(self, mock_config, mock_put) -> None: - with pytest.raises(ValidationError, match="Local metrics"): - LocalEvaluator(name="local", scorer_fn=lambda trace: 1.0).set_scope(project_ids=["a"]) + # Given: a local-only evaluator + local = LocalEvaluator(name="local", scorer_fn=lambda trace: 1.0) + + # When/Then: set_scope refuses before any request + with pytest.raises(ValidationError, match="Local evaluators"): + local.set_scope(project_ids=["a"]) mock_put.sync.assert_not_called() def test_missing_id_is_rejected(self, mock_config, mock_put) -> None: + # Given: an evaluator that was never created + evaluator = LlmEvaluator(name="ev", prompt="p") + + # When/Then: set_scope refuses before any request with pytest.raises(ValueError, match="ID is not set"): - LlmEvaluator(name="ev", prompt="p").set_scope(project_ids=["a"]) + evaluator.set_scope(project_ids=["a"]) mock_put.sync.assert_not_called() @pytest.mark.parametrize("state", [SyncState.DELETED, SyncState.FAILED_SYNC]) def test_deleted_and_failed_sync_are_rejected(self, mock_config, mock_put, state) -> None: + # Given: an evaluator in a state that cannot be changed evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) evaluator._set_state(state) + + # When/Then: set_scope refuses before any request with pytest.raises(ValueError, match=r"deleted|FAILED_SYNC"): evaluator.set_scope(project_ids=["a"]) mock_put.sync.assert_not_called() @pytest.mark.parametrize("kwargs", [{"project_ids": ["a"]}, {"is_global": True}], ids=["scoped", "global"]) def test_403_is_translated_and_marks_failed(self, mock_config, mock_put, kwargs) -> None: + # Given: a synced evaluator whose scope change the server refuses evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) mock_put.sync.side_effect = _forbidden() + + # When: its scope is changed with pytest.raises(EvaluatorPermissionError) as exc_info: evaluator.set_scope(**kwargs) + + # Then: the scope-change hint is raised and the state is FAILED_SYNC _assert_permission_error(exc_info.value, SCOPE_HINT) - assert "making it global needs the org admin role" in str(exc_info.value) assert evaluator.sync_state == SyncState.FAILED_SYNC def test_404_stays_not_found(self, mock_config, mock_put) -> None: + # Given: the server answers 404 (it hides evaluators outside the caller's projects) evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) mock_put.sync.side_effect = NotFoundError(404, b"gone") + + # When: its scope is changed with pytest.raises(NotFoundError) as exc_info: evaluator.set_scope(project_ids=["a"]) + + # Then: it stays a NotFoundError, never a permission error, and the state is FAILED_SYNC assert not isinstance(exc_info.value, EvaluatorPermissionError) assert evaluator.sync_state == SyncState.FAILED_SYNC def test_validation_error_response_raises_api_error(self, mock_config, mock_put) -> None: + # Given: the server answers with a validation error evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) mock_put.sync.return_value = HTTPValidationError(detail="unknown project") + + # When/Then: an APIError carries the detail with pytest.raises(APIError, match="unknown project"): evaluator.set_scope(project_ids=["a"]) def test_empty_response_raises_api_error(self, mock_config, mock_put) -> None: + # Given: the server returns nothing evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) mock_put.sync.return_value = None + + # When/Then: an APIError reports the empty response with pytest.raises(APIError, match="empty response"): evaluator.set_scope(project_ids=["a"]) From f08e5a99f5227dc6d1d0cfab2a2c1d69a3b2d09d Mon Sep 17 00:00:00 2001 From: quinn-galileo Date: Mon, 19 Oct 2026 12:56:53 -0400 Subject: [PATCH 7/9] fix: document decision create permission errors Co-Authored-By: Claude Opus 5.5 (1M context) --- src/splunk_ao/evaluator.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/splunk_ao/evaluator.py b/src/splunk_ao/evaluator.py index 82b33780..d1cce9c1 100644 --- a/src/splunk_ao/evaluator.py +++ b/src/splunk_ao/evaluator.py @@ -1122,7 +1122,8 @@ def create(self) -> DecisionEvaluator: ``project_ids``, or ``is_global=False`` with no project). Scope errors are ``ValidationError``, not ``ValueError``. EvaluatorPermissionError: If the API answers 403: org admins only for a global evaluator, owner or - editor role on the projects for a scoped one. + editor role on the projects for a scoped one. Decision evaluators may also not be enabled for the + organization. A 403 on the version request deletes the new scorer before the error is raised. NotFoundError: If the current project is set but cannot be found. ValueError: If an argument is invalid or the API answers with a validation error. SplunkAOAPIError: For HTTP errors. A scorer created before a clear rejection is deleted. From 67449e587f8b9762ae05231169d3df17a798deca Mon Sep 17 00:00:00 2001 From: quinn-galileo Date: Tue, 20 Oct 2026 12:52:58 -0400 Subject: [PATCH 8/9] chore: correct README note on not-found errors Co-Authored-By: Claude Opus 5.5 (1M context) --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index ceb55a5d..a4c638f5 100644 --- a/README.md +++ b/README.md @@ -651,7 +651,8 @@ except EvaluatorPermissionError as error: `EvaluatorPermissionError` subclasses `ForbiddenError`. `Evaluator.get(name=...)` returns `None` when the evaluator does not exist or is not shared with your projects. The server answers 404 rather than 403 for an evaluator outside -your projects on some paths, so `update`, `delete` and `set_scope` can raise `NotFoundError` in that case. +your projects on some paths. In that case `update` and `set_scope` can raise `NotFoundError`, and `delete` (which goes +by name) raises a `ValueError` saying it was not found, or not shared with your projects. ### Sessions From e181917a3507eba17ef90369f19b09471a4a3d84 Mon Sep 17 00:00:00 2001 From: quinn-galileo Date: Tue, 20 Oct 2026 12:56:56 -0400 Subject: [PATCH 9/9] fix: delete an evaluator by id and align not-found notes Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 5 ++- README.md | 5 ++- src/splunk_ao/evaluator.py | 19 +++++++--- src/splunk_ao/evaluators.py | 1 + tests/test_evaluator.py | 17 +++++---- tests/test_evaluator_permissions.py | 57 +++++++++++++++++++++++++++-- 6 files changed, 85 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 25b1f525..4a3a12ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -76,7 +76,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `Evaluators.delete_evaluator()` and `Evaluator.refresh()` now say "not found, or not shared with your projects" when the evaluator is missing, since the server hides evaluators outside the caller's projects. `Evaluator.get()` - still returns `None` in that case. + still returns `None` in that case. Calls by id (`update()`, `delete()`, `set_scope()`) may raise `NotFoundError` + instead; `delete_evaluator(name=...)` and `Evaluator.delete_by_name()` raise `ValueError`. +- `Evaluator.delete()` now deletes by id. It used to delete every evaluator that shared its name, which could also hit + one the caller had no permission to delete. - Creating an LLM or code evaluator no longer fails with an `AttributeError` when the scorer request is answered with a validation error; the detail is raised as a `ValueError`, as it already was for decision evaluators. - The API client now knows the permission and enum values that current servers return, so reading projects and diff --git a/README.md b/README.md index a4c638f5..9b20886e 100644 --- a/README.md +++ b/README.md @@ -651,8 +651,9 @@ except EvaluatorPermissionError as error: `EvaluatorPermissionError` subclasses `ForbiddenError`. `Evaluator.get(name=...)` returns `None` when the evaluator does not exist or is not shared with your projects. The server answers 404 rather than 403 for an evaluator outside -your projects on some paths. In that case `update` and `set_scope` can raise `NotFoundError`, and `delete` (which goes -by name) raises a `ValueError` saying it was not found, or not shared with your projects. +your projects on some paths. In that case `update`, `delete` and `set_scope` (which go by id) can raise `NotFoundError`, +while `delete_evaluator(name=...)` and `Evaluator.delete_by_name` (which go by name) raise a `ValueError` saying the +evaluator was not found, or not shared with your projects. ### Sessions diff --git a/src/splunk_ao/evaluator.py b/src/splunk_ao/evaluator.py index d1cce9c1..140201a3 100644 --- a/src/splunk_ao/evaluator.py +++ b/src/splunk_ao/evaluator.py @@ -35,6 +35,7 @@ from splunk_ao.resources.api.data import ( create_code_scorer_version_scorers_scorer_id_version_code_post, create_scorers_post, + delete_scorer_scorers_scorer_id_delete, get_validate_code_scorer_task_result_scorers_code_validate_task_id_get, set_scorer_scope_scorers_scorer_id_scope_put, update_scorers_scorer_id_patch, @@ -662,14 +663,17 @@ def delete(self) -> None: """ Delete this metric. - Only works for server-side metrics. Local metrics don't need deletion. + Only works for server-side metrics. Local metrics don't need deletion. Deletes by id, so a different + evaluator that shares this one's name is not touched. Raises ------ ValidationError: If this is a local metric. - ValueError: If the metric is not synced, or is not found (or not shared with your projects). + ValueError: If the metric ID is not set, or the API answers with a validation error or empty response. EvaluatorPermissionError: If the API answers 403: deleting needs the owner or editor role on one of the evaluator's projects (org admin for a global evaluator). + NotFoundError: If the API answers 404. The server also answers 404 for an evaluator that exists but + isn't shared with your projects. Examples -------- @@ -684,8 +688,13 @@ def delete(self) -> None: try: logger.info(f"Evaluator.delete: id='{self.id}' name='{self.name}' - started") - metrics_service = Evaluators() - metrics_service.delete_evaluator(name=self.name) + config = SplunkAOConfig.get() + with _translate_forbidden(_edit_denied_hint("Deleting")): + response = delete_scorer_scorers_scorer_id_delete.sync(scorer_id=self.id, client=config.api_client) + if isinstance(response, HTTPValidationError): + raise ValueError(response.detail) + if response is None: + raise ValueError("Failed to delete metric.") self._set_state(SyncState.DELETED) logger.info(f"Evaluator.delete: id='{self.id}' - completed") except Exception as e: @@ -724,7 +733,7 @@ def refresh(self) -> None: retrieved_scorer = next((s for s in scorers if s.id == self.id), None) if retrieved_scorer is None: - raise ValueError(f"Evaluator with id '{self.id}' not found, or not shared with your projects") + raise ValueError(f"Evaluator with id '{self.id}' not found, or not shared with your projects.") self._populate_from_scorer_response(retrieved_scorer) self._set_state(SyncState.SYNCED) diff --git a/src/splunk_ao/evaluators.py b/src/splunk_ao/evaluators.py index d8e4b71c..cc1274fe 100644 --- a/src/splunk_ao/evaluators.py +++ b/src/splunk_ao/evaluators.py @@ -95,6 +95,7 @@ def __init__(self, scorer_id: str, cause: Exception) -> None: ) +# E2E tests match the phrases "org admin" and "owner or editor" in these hints, so keep them. _GLOBAL_CREATE_DENIED_HINT = ( "Only org admins can create global evaluators. Pass project_ids=[...] to scope the evaluator to your projects, " "or set SPLUNK_AO_PROJECT / SPLUNK_AO_PROJECT_ID so it defaults to one." diff --git a/tests/test_evaluator.py b/tests/test_evaluator.py index 251105f7..42f81298 100644 --- a/tests/test_evaluator.py +++ b/tests/test_evaluator.py @@ -502,17 +502,20 @@ def test_list_with_scorer_types_filter(self, mock_scorers_class: MagicMock, rese class TestMetricDelete: """Test suite for Evaluator.delete() method.""" - @patch("splunk_ao.evaluator.Evaluators") + @patch("splunk_ao.evaluator.SplunkAOConfig.get") + @patch("splunk_ao.evaluator.delete_scorer_scorers_scorer_id_delete") @patch("splunk_ao.evaluator.Scorers") def test_delete_removes_metric( - self, mock_scorers_class: MagicMock, mock_metrics_class: MagicMock, reset_configuration: None + self, + mock_scorers_class: MagicMock, + mock_delete: MagicMock, + mock_config_get: MagicMock, + reset_configuration: None, ) -> None: - """Test delete() removes the metric.""" + """Test delete() removes the metric by id.""" mock_scorers_service = MagicMock() mock_scorers_class.return_value = mock_scorers_service - - mock_metrics_service = MagicMock() - mock_metrics_class.return_value = mock_metrics_service + mock_delete.sync.return_value = {"message": "deleted"} metric_id = str(uuid4()) mock_scorer = MagicMock() @@ -533,7 +536,7 @@ def test_delete_removes_metric( metric = Evaluator.get(id=metric_id) metric.delete() - mock_metrics_service.delete_evaluator.assert_called_once_with(name="Test Evaluator") + mock_delete.sync.assert_called_once_with(scorer_id=metric_id, client=mock_config_get.return_value.api_client) assert metric.sync_state == SyncState.DELETED def test_delete_raises_error_for_local_only(self, reset_configuration: None) -> None: diff --git a/tests/test_evaluator_permissions.py b/tests/test_evaluator_permissions.py index 0b0cc433..49e84c26 100644 --- a/tests/test_evaluator_permissions.py +++ b/tests/test_evaluator_permissions.py @@ -350,12 +350,11 @@ def test_delete_by_name_403_is_translated(self, mock_list, mock_delete) -> None: # Then: the delete hint is raised _assert_permission_error(exc_info.value, DELETE_HINT) - @patch("splunk_ao.evaluators.delete_scorer_scorers_scorer_id_delete") - @patch("splunk_ao.scorers.Scorers.list") - def test_instance_delete_403_is_translated_and_marks_failed(self, mock_list, mock_delete) -> None: + @patch("splunk_ao.evaluator.delete_scorer_scorers_scorer_id_delete") + @patch("splunk_ao.evaluator.SplunkAOConfig.get") + def test_instance_delete_403_is_translated_and_marks_failed(self, mock_config, mock_delete) -> None: # Given: a synced evaluator whose delete the server refuses evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) - mock_list.return_value = [_scorer_response(str(uuid4()))] mock_delete.sync.side_effect = _forbidden() # When: it is deleted @@ -366,6 +365,56 @@ def test_instance_delete_403_is_translated_and_marks_failed(self, mock_list, moc _assert_permission_error(exc_info.value, DELETE_HINT) assert evaluator.sync_state == SyncState.FAILED_SYNC + @patch("splunk_ao.evaluator.delete_scorer_scorers_scorer_id_delete") + @patch("splunk_ao.evaluator.SplunkAOConfig.get") + @patch("splunk_ao.scorers.Scorers.list") + def test_instance_delete_sends_one_delete_for_its_own_id(self, mock_list, mock_config, mock_delete) -> None: + # Given: a synced evaluator and a same-named sibling that the name lookup would also return + evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) + mock_list.return_value = [_scorer_response(evaluator.id), _scorer_response(str(uuid4()))] + mock_delete.sync.return_value = {"message": "deleted"} + + # When: the evaluator is deleted + evaluator.delete() + + # Then: exactly one DELETE goes out, for its own id, and the sibling is untouched + mock_delete.sync.assert_called_once() + assert mock_delete.sync.call_args.kwargs["scorer_id"] == evaluator.id + mock_list.assert_not_called() + assert evaluator.sync_state == SyncState.DELETED + + @patch("splunk_ao.evaluator.delete_scorer_scorers_scorer_id_delete") + @patch("splunk_ao.evaluator.SplunkAOConfig.get") + def test_instance_delete_404_stays_not_found(self, mock_config, mock_delete) -> None: + # Given: the server answers 404 (it hides evaluators outside the caller's projects) + evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) + mock_delete.sync.side_effect = NotFoundError(404, b"gone") + + # When: the evaluator is deleted + with pytest.raises(NotFoundError) as exc_info: + evaluator.delete() + + # Then: it stays a NotFoundError, never a permission error, and the state is FAILED_SYNC + assert not isinstance(exc_info.value, EvaluatorPermissionError) + assert evaluator.sync_state == SyncState.FAILED_SYNC + + @patch("splunk_ao.evaluator.delete_scorer_scorers_scorer_id_delete") + @patch("splunk_ao.evaluator.SplunkAOConfig.get") + @pytest.mark.parametrize( + ("response", "message"), + [(HTTPValidationError(detail="bad id"), "bad id"), (None, "Failed to delete")], + ids=["validation_error", "empty_response"], + ) + def test_instance_delete_rejects_a_bad_response(self, mock_config, mock_delete, response, message) -> None: + # Given: the server answers with a validation error or nothing + evaluator = _synced(LlmEvaluator(name="ev", prompt="p")) + mock_delete.sync.return_value = response + + # When/Then: a ValueError is raised and the state is FAILED_SYNC + with pytest.raises(ValueError, match=message): + evaluator.delete() + assert evaluator.sync_state == SyncState.FAILED_SYNC + @patch("splunk_ao.evaluators.delete_scorer_scorers_scorer_id_delete") @patch("splunk_ao.scorers.Scorers.list") def test_delete_404_stays_not_found(self, mock_list, mock_delete) -> None: