Conversation
|
|
||
| # Start backend with uvicorn | ||
| # Default to no initializers | ||
| if initializers is None: |
There was a problem hiding this comment.
Chicken/egg, but pretty quickly it'd be nice to update this to use the config, and we can just get rid of initializers here: #1343
Maybe we should check in with Victor and use it to begin with?
There was a problem hiding this comment.
Right. Don't want to block on that but that was my feeling, too, while reading that PR.
| """ | ||
| Attack-related request and response models. | ||
|
|
||
| All interactions in the UI are modeled as "attacks" - including manual conversations. |
There was a problem hiding this comment.
One thing to think about is that we likely want the same types of separations we have in workflows. I could also see us wanting to benchmark things or to setup prompt generation.
But for now this is probably good to get started.
There was a problem hiding this comment.
That will probably be different since there are no attack results, though.
| """ | ||
|
|
||
| piece_id: str = Field(..., description="Unique piece identifier") | ||
| data_type: str = Field(default="text", description="Data type: 'text', 'image', 'audio', 'video', etc.") |
There was a problem hiding this comment.
wouldn't we need both original_value_datatype and converted_value_data_type?
| original_value: Optional[str] = Field(default=None, description="Original value before conversion") | ||
| original_value_mime_type: Optional[str] = Field(default=None, description="MIME type of original value") | ||
| converted_value: str = Field(..., description="Converted value (text or base64 for media)") | ||
| converted_value_mime_type: Optional[str] = Field(default=None, description="MIME type of converted value") |
There was a problem hiding this comment.
I'm guessing we need mime type to upload/download content. If we need it here I could see us needing it elsewhere. Should we add it to MessagePiece? Should we make this a thinner wrapper of that?
There was a problem hiding this comment.
It's for adding files (imagine any modality other than text).
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive backend REST APIs to support upcoming frontend development, implementing an attack-centric design where all user interactions (including manual conversations) are modeled as "attacks". The implementation includes service layers, API routes, Pydantic models, error handling middleware, registries for managing target/converter instances, a CLI tool, and comprehensive test coverage.
Changes:
- Adds three main services (AttackService, ConverterService, TargetService) for managing attacks, converters, and targets
- Implements REST API routes for attacks, targets, converters, labels, health, and version endpoints
- Creates Pydantic models for request/response validation with RFC 7807 error handling
- Adds instance registries for targets and converters with singleton pattern
- Implements
pyrit_backendCLI command for starting the server with initializer support - Includes 900+ lines of comprehensive unit tests for all services and routes
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| pyrit/backend/services/attack_service.py | Service layer for managing attack lifecycle, messages, and scoring (586 lines) |
| pyrit/backend/services/converter_service.py | Service for managing converter instances and previewing conversions (303 lines) |
| pyrit/backend/services/target_service.py | Service for managing target instance creation and retrieval (187 lines) |
| pyrit/backend/routes/attacks.py | REST API endpoints for attack CRUD operations and messaging (249 lines) |
| pyrit/backend/routes/targets.py | REST API endpoints for target instance management (101 lines) |
| pyrit/backend/routes/converters.py | REST API endpoints for converter instances and preview (134 lines) |
| pyrit/backend/routes/labels.py | REST API endpoint for retrieving filter label options (88 lines) |
| pyrit/backend/models/attacks.py | Pydantic models for attack requests/responses (201 lines) |
| pyrit/backend/models/targets.py | Pydantic models for target instances (52 lines) |
| pyrit/backend/models/converters.py | Pydantic models for converter instances and preview (98 lines) |
| pyrit/backend/models/common.py | Common models including RFC 7807 error responses and sensitive field filtering (93 lines) |
| pyrit/backend/middleware/error_handlers.py | RFC 7807 compliant error handler middleware (182 lines) |
| pyrit/registry/instance_registries/target_registry.py | Registry for managing target instances (88 lines) |
| pyrit/registry/instance_registries/converter_registry.py | Registry for managing converter instances (108 lines) |
| pyrit/cli/pyrit_backend.py | CLI command for starting the backend server with initialization support (217 lines) |
| tests/unit/backend/*.py | Comprehensive unit tests for all services, routes, models, and error handlers (2000+ lines) |
| pyrit/backend/main.py | Updated to register new routes and error handlers |
| pyproject.toml | Adds pyrit_backend CLI entry point and documentation exemption |
| frontend/dev.py | Updated to use pyrit_backend CLI instead of direct uvicorn |
| async def list_converters(self) -> ConverterInstanceListResponse: | ||
| """ | ||
| List all converter instances. | ||
|
|
||
| Returns: | ||
| ConverterInstanceListResponse containing all registered converters. | ||
| """ | ||
| items = [ | ||
| self._build_instance_from_object(name, obj) for name, obj in self._registry.get_all_instances().items() | ||
| ] | ||
| return ConverterInstanceListResponse(items=items) | ||
|
|
||
| async def get_converter(self, converter_id: str) -> Optional[ConverterInstance]: | ||
| """ | ||
| Get a converter instance by ID. | ||
|
|
||
| Returns: | ||
| ConverterInstance if found, None otherwise. | ||
| """ | ||
| obj = self._registry.get_instance_by_name(converter_id) | ||
| if obj is None: | ||
| return None | ||
| return self._build_instance_from_object(converter_id, obj) | ||
|
|
||
| def get_converter_object(self, converter_id: str) -> Optional[Any]: | ||
| """ | ||
| Get the actual converter object. | ||
|
|
||
| Returns: | ||
| The PromptConverter object if found, None otherwise. | ||
| """ | ||
| return self._registry.get_instance_by_name(converter_id) | ||
|
|
||
| async def create_converter(self, request: CreateConverterRequest) -> CreateConverterResponse: | ||
| """ | ||
| Create a new converter instance from API request. | ||
|
|
||
| Instantiates the converter with the given type and params, | ||
| then registers it in the registry. | ||
|
|
||
| Args: | ||
| request: The create converter request with type and params. | ||
|
|
||
| Returns: | ||
| CreateConverterResponse with the new converter's details. | ||
|
|
||
| Raises: | ||
| ValueError: If the converter type is not found. | ||
| """ | ||
| converter_id = str(uuid.uuid4()) | ||
|
|
||
| # Resolve any converter references in params and instantiate | ||
| params = self._resolve_converter_params(request.params) | ||
| converter_class = self._get_converter_class(request.type) | ||
| converter_obj = converter_class(**params) | ||
| self._registry.register_instance(converter_obj, name=converter_id) | ||
|
|
||
| return CreateConverterResponse( | ||
| converter_id=converter_id, | ||
| type=request.type, | ||
| display_name=request.display_name, | ||
| params=request.params, | ||
| ) | ||
|
|
||
| async def preview_conversion(self, request: ConverterPreviewRequest) -> ConverterPreviewResponse: |
There was a problem hiding this comment.
All async functions and methods MUST end with _async suffix according to PyRIT coding guidelines. The following async functions in this file are missing the _async suffix:
list_convertersget_convertercreate_converterpreview_conversion
These should be renamed to:
list_converters_asyncget_converter_asynccreate_converter_asyncpreview_conversion_async
| async def list_targets(self) -> TargetListResponse: | ||
| """ | ||
| List all target instances. | ||
|
|
||
| Returns: | ||
| TargetListResponse containing all registered targets. | ||
| """ | ||
| items = [ | ||
| self._build_instance_from_object(name, obj) for name, obj in self._registry.get_all_instances().items() | ||
| ] | ||
| return TargetListResponse(items=items) | ||
|
|
||
| async def get_target(self, target_id: str) -> Optional[TargetInstance]: | ||
| """ | ||
| Get a target instance by ID. | ||
|
|
||
| Returns: | ||
| TargetInstance if found, None otherwise. | ||
| """ | ||
| obj = self._registry.get_instance_by_name(target_id) | ||
| if obj is None: | ||
| return None | ||
| return self._build_instance_from_object(target_id, obj) | ||
|
|
||
| def get_target_object(self, target_id: str) -> Optional[Any]: | ||
| """ | ||
| Get the actual target object for use in attacks. | ||
|
|
||
| Returns: | ||
| The PromptTarget object if found, None otherwise. | ||
| """ | ||
| return self._registry.get_instance_by_name(target_id) | ||
|
|
||
| async def create_target(self, request: CreateTargetRequest) -> CreateTargetResponse: |
There was a problem hiding this comment.
All async functions and methods MUST end with _async suffix according to PyRIT coding guidelines. The following async functions in this file are missing the _async suffix:
list_targetsget_targetcreate_target
These should be renamed to:
list_targets_asyncget_target_asynccreate_target_async
| pagination=PaginationInfo(limit=limit, has_more=has_more, next_cursor=next_cursor, prev_cursor=cursor), | ||
| ) | ||
|
|
||
| async def get_attack(self, attack_id: str) -> Optional[AttackSummary]: |
There was a problem hiding this comment.
Functions with more than 1 parameter MUST use * after self to enforce keyword-only arguments according to PyRIT coding guidelines. The function get_attack has 2 parameters (self, attack_id) but doesn't enforce keyword-only arguments.
Change to:
async def get_attack(self, *, attack_id: str) -> Optional[AttackSummary]:| def test_get_attack_service_returns_attack_service(self) -> None: | ||
| """Test that get_attack_service returns an AttackService instance.""" | ||
| # Reset singleton for clean test | ||
| import pyrit.backend.services.attack_service as module |
There was a problem hiding this comment.
Module 'pyrit.backend.services.attack_service' is imported with both 'import' and 'import from'.
| def test_get_attack_service_returns_same_instance(self) -> None: | ||
| """Test that get_attack_service returns the same instance.""" | ||
| # Reset singleton for clean test | ||
| import pyrit.backend.services.attack_service as module |
There was a problem hiding this comment.
Module 'pyrit.backend.services.attack_service' is imported with both 'import' and 'import from'.
|
|
||
| import pytest | ||
|
|
||
| import pyrit.backend.services.converter_service as converter_service_module |
There was a problem hiding this comment.
Module 'pyrit.backend.services.converter_service' is imported with both 'import' and 'import from'.
|
|
||
| def test_get_target_service_returns_target_service(self) -> None: | ||
| """Test that get_target_service returns a TargetService instance.""" | ||
| import pyrit.backend.services.target_service as module |
There was a problem hiding this comment.
Module 'pyrit.backend.services.target_service' is imported with both 'import' and 'import from'.
|
|
||
| def test_get_target_service_returns_same_instance(self) -> None: | ||
| """Test that get_target_service returns the same instance.""" | ||
| import pyrit.backend.services.target_service as module |
There was a problem hiding this comment.
Module 'pyrit.backend.services.target_service' is imported with both 'import' and 'import from'.
| # Copyright (c) Microsoft Corporation. | ||
| # Licensed under the MIT license. | ||
|
|
||
| """ |
There was a problem hiding this comment.
Concern: Model Duplication/Drift and Confusion
A lot of these are really similar to our core models (MessagePiece, Message, Score, AttackSummary). Additionally, converters and targets here are really similar to the corresponding Identifiers.
Right now, the translation logic (in attack_service.py) is fragile. A name change breaks things, and things like type could all be different (e.g. score_value is float in the backend but a string in the model)
I also think this would be easier to use if all the same fields are available. It could be confusing to program here and not have access to converted_data_type. As a new user I'd be asking myself "what is converted_type and how does it map?". I think it would be really useful to be able to access all the model pieces in the same ways.
Proposal
Could we define the Pydantic models with the same field names and drift detection? E.g. use from_attributes=True
# pyrit/backend/models/message_piece.py
class MessagePieceSchema(BaseModel):
model_config = ConfigDict(from_attributes=True)
# Same names as domain MessagePiece
id: Optional[str] = None # serialized from UUID
role: Literal["system", "user", "assistant", "simulated_assistant"]
original_value: str
converted_value: Optional[str] = None
original_value_data_type: str = "text"
converted_value_data_type: Optional[str] = None
response_error: str = "none"
sequence: int = -1
conversation_id: Optional[str] = None
# ... all 25 fields with same names ...
# API-only extras
friendly_name: Optional[str] = None
_EXTRA_FIELDS = {"friendly_name"}
# Potentially add this as an abstract method in a base class to ensure we add it in all backend models
@classmethod
def from_model(cls, obj, **extras):
instance = cls.model_validate(obj)
for k, v in extras.items():
setattr(instance, k, v)
return instance
Apply the same paradigm to identifiers:
# pyrit/backend/models/converters.py
class ConverterIdentifierSchema(BaseModel):
model_config = ConfigDict(from_attributes=True)
# Same fields as domain ConverterIdentifier
__type__: str
__module__: str
id: Optional[str] = None
supported_input_types: Optional[tuple] = None
supported_output_types: Optional[tuple] = None
sub_converter_identifiers: Optional[List["ConverterIdentifierSchema"]] = None
# ... etc
# API extras
display_name: Optional[str] = None
Add some kind of drift detection test. We could at runtime or even tests I'd be happy with
# tests/unit/backend/test_model_mirrors.py
class TestMessagePieceMirror:
def test_all_domain_fields_present(self):
domain_fields = get_init_param_names(MessagePiece)
schema_fields = set(MessagePieceSchema.model_fields.keys())
extra_fields = MessagePieceSchema._EXTRA_FIELDS
missing = domain_fields - (schema_fields - extra_fields)
assert not missing, f"Schema missing domain fields: {missing}"
Then instead of attack_service, translate with from_model
# Before (in attack_service.py)
pieces = [MessagePiece(piece_id=str(p.id), data_type=p.converted_value_data_type, ...)]
# After
pieces = [MessagePieceSchema.from_model(p) for p in msg.message_pieces]
2 participants
Description
Adding backend APIs to support upcoming frontend development. This is based on an initial proposal and review.
Tests and Documentation
Includes tests for all APIs.