Fix #2495: discriminate authorization vs. value errors at /mutate boundary

orchestrator/routes/contracts.py

@@ -362,11 +362,16 @@ def mutate_contract(identifier: str) -> tuple[Response, int]:
                     "role": role.value,
                     "field_path": field_path,
                     "error": result.message,
+                    "error_kind": result.error_kind,
                 },
             )
+            # 403 only for authorization rejections; value/path errors
+            # are 400 so a client doesn't retry them as if a different
+            # role might succeed (#2495).
+            status_code = 403 if result.error_kind == "authorization" else 400
             return _error(
                 result.message,
-                status_code=403,
+                status_code=status_code,
                 details={"role": role.value, "field_path": field_path},
             )
 

orchestrator/tests/test_contracts_routes.py

@@ -171,7 +171,56 @@ def test_role_validation_enforced(self, client, fake_worktree):
                 "new_value": "abc1234",
             },
         )
-        assert response.status_code in (403, 400)
+        assert response.status_code == 403
+
+    def test_invalid_path_returns_400(self, client, fake_worktree):
+        """#2495: out-of-range index is a value error, not authorization.
+
+        Implementer is authorized to mutate ``phases.*.status``, but the
+        seeded contract has no phases — index 0 is out of range and
+        ``apply_mutation`` translates the ``IndexError`` to ``MutationResult``
+        with ``error_kind="value"``.  The route returns 400 so a client
+        does not retry as a different role.
+        """
+        pipeline_id, worktree = fake_worktree
+        _seed_contract(worktree, pipeline_id)
+
+        response = self._mutate(
+            client,
+            pipeline_id,
+            role="implementer",
+            body_overrides={
+                "field_path": "phases.0.status",
+                "new_value": "complete",
+            },
+        )
+        assert response.status_code == 400
+        assert "Failed to apply mutation" in json.loads(response.data)["message"]
+
+    def test_invalid_value_returns_400(self, client, fake_worktree):
+        """#2495: out-of-domain value (e.g. arbitrary string for a
+        ``PipelinePhase`` enum) is a value error, not authorization.
+
+        Reviewer is authorized to mutate ``current_phase``, but pydantic's
+        ``validate_assignment=True`` (added for #2465) raises
+        ``ValidationError`` for ``"garbage"``.  ``apply_mutation``
+        translates that to ``MutationResult`` with ``error_kind="value"``,
+        and the route returns 400.
+        """
+        pipeline_id, worktree = fake_worktree
+        _seed_contract(worktree, pipeline_id)
+
+        response = self._mutate(
+            client,
+            pipeline_id,
+            role="reviewer",
+            body_overrides={
+                "field_path": "current_phase",
+                "new_value": "garbage",
+            },
+        )
+        assert response.status_code == 400
+        assert "Invalid value for current_phase" in json.loads(response.data)["message"]
 
 
 class TestValidateMutation:

shared/egg_contracts/__init__.py

@@ -221,6 +221,7 @@
     normalize_path,
 )
 from .validator import (
+    MutationErrorKind,
     MutationResult,
     ValidationResult,
     apply_mutation,
@@ -253,6 +254,7 @@
     # Roles
     "FIELD_OWNERSHIP",
     "IssueInfo",
+    "MutationErrorKind",
     "MutationResult",
     "Phase",
     "PhaseStatus",

shared/egg_contracts/validator.py

@@ -6,7 +6,7 @@
 """
 
 from dataclasses import dataclass
-from typing import Any
+from typing import Any, Literal
 
 from pydantic import ValidationError
 
@@ -25,14 +25,24 @@ class ValidationResult:
     required_role: str | None = None
 
 
+MutationErrorKind = Literal["authorization", "value"]
+
+
 @dataclass
 class MutationResult:
-    """Result of applying a mutation."""
+    """Result of applying a mutation.
+
+    ``error_kind`` discriminates failure causes for the route boundary
+    (#2495): ``"authorization"`` is a role-permission rejection (HTTP
+    403); ``"value"`` is a malformed path or out-of-domain value (HTTP
+    400/422). ``None`` on success.
+    """
 
     success: bool
     message: str
     contract: Contract | None = None
     audit_entry: AuditEntry | None = None
+    error_kind: MutationErrorKind | None = None
 
 
 def validate_mutation(
@@ -103,6 +113,7 @@ def apply_mutation(
         return MutationResult(
             success=False,
             message=validation.message,
+            error_kind="authorization",
         )
 
     # Get the old value and apply the mutation
@@ -119,6 +130,7 @@ def apply_mutation(
         return MutationResult(
             success=False,
             message=f"Failed to apply mutation: {e}",
+            error_kind="value",
         )
     except ValidationError as e:
         # ``Contract.model_config = ConfigDict(validate_assignment=True)``
@@ -130,6 +142,7 @@ def apply_mutation(
         return MutationResult(
             success=False,
             message=f"Invalid value for {field_path}: {e}",
+            error_kind="value",
         )
 
     # Create audit entry. ``current_phase`` mutations get the dedicated

tests/shared/egg_contracts/test_validator.py

@@ -147,6 +147,9 @@ def test_apply_invalid_mutation_rejected(self, sample_contract):
         assert "reviewer" in result.message.lower()
         assert result.contract is None
         assert result.audit_entry is None
+        # #2495: role-permission rejections must surface as
+        # ``error_kind="authorization"`` so the route boundary returns 403.
+        assert result.error_kind == "authorization"
 
     def test_implementer_can_set_task_status(self, sample_contract):
         """Test that implementer can mark task complete (shared ownership)."""
@@ -309,6 +312,32 @@ def test_invalid_enum_value_returns_failed_mutation(self, sample_contract):
         assert "current_phase" in result.message
         # The original value is unchanged.
         assert sample_contract.current_phase is PipelinePhase.REFINE
+        # #2495: out-of-domain values must surface as ``error_kind="value"``
+        # so the route boundary returns 400 (not 403).
+        assert result.error_kind == "value"
+
+    def test_invalid_path_returns_failed_mutation(self, sample_contract):
+        """#2495: out-of-range index path errors return ``error_kind="value"``.
+
+        Implementer is authorized to mutate ``phases.*.tasks.*.commit`` (the
+        role check passes), but ``phases.99.tasks.0.commit`` raises
+        ``IndexError`` from inside ``_set_value``.  ``apply_mutation`` must
+        catch that and surface it through ``MutationResult`` with
+        ``error_kind="value"`` so the route boundary returns 400 instead of
+        misclassifying the failure as authorization (403).
+        """
+        result = apply_mutation(
+            contract=sample_contract,
+            role=Role.IMPLEMENTER,
+            actor="james-in-a-box",
+            field_path="phases.99.tasks.0.commit",
+            new_value="abc1234",
+        )
+
+        assert result.success is False
+        assert result.contract is None
+        assert result.audit_entry is None
+        assert result.error_kind == "value"
 
     def test_non_current_phase_field_still_emits_update_action(self, sample_contract):
         """Non-current_phase fields continue to emit AuditAction.UPDATE."""