FEAT: Supporting Converter Configuration on REST API - #2536
FEAT: Supporting Converter Configuration on REST API#2536Richard Lundeen (rlundeen2) wants to merge 1 commit into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: dad4cb22-3dd3-41d5-b874-2df319502396
| Raises: | ||
| ValueError: If converter fields conflict, cannot run, or contain an out-of-range request index. | ||
| """ | ||
| if self.converter_ids is not None and self.request_converter_configurations is not None: |
There was a problem hiding this comment.
should we use truthiness here instead of is not None ie check :
raise ValueError()
The current is not None check treats converter_ids=[] as an active pipeline. This causes previously valid requests to fail, such as send=False with an empty converter list. It also conflicts with request_converter_configurations, even though the legacy list has nothing in it.
if we swap to check converter_ids and conifgurations (rather than is not none), we only reject request when both lists actually contain configurations
| new_item="AddMessageRequest.request_converter_configurations", | ||
| removed_in="1.3.0", | ||
| ) | ||
| converters = get_converter_service().get_converter_objects_for_ids(converter_ids=request.converter_ids) |
There was a problem hiding this comment.
i think add second check "if request.converter_ids" before we do this:
The current check assumes these two statements mean the same thing:
- The caller supplied
converter_idsand 2. The caller supplied converters that should run.
They are not equivalent when the caller sends: "converter_ids": []
By adding the 2nd check, we can handle the two concerns separately:
# The deprecated field was supplied, so warn.
if request.converter_ids:
# The list contains converters, so execute the legacy pipeline.
Rn, an empty list triggers the warning and immediately returns an empty pipeline, preventing the structured configuration from being used. With the recommended change, it would still warn but then continue to request_converter_configurations. Separating those decisions would prevent an empty legacy field from overriding the new pipeline.
| if self.converter_ids is not None and self.request_converter_configurations is not None: | ||
| raise ValueError("converter_ids and request_converter_configurations cannot both be provided") | ||
|
|
||
| has_converter_configurations = any( |
There was a problem hiding this comment.
think you could change to
self.converter_ids
or self.request_converter_configurations
or self.response_converter_configurations
)
so that converter_ids=[] doesn't count as active converter pipeline since there's nothing to run
| existing=attack_id.request_converters, | ||
| additions=request_converter_ids, | ||
| ) | ||
| merged_response_converters = self._merge_converter_identifiers( |
There was a problem hiding this comment.
now that response converters are persisted here, should the existing converter history APIs also be updated to consider them? might be out of scope of this PR or entirely but currently AttackSummary.converters , has_converters , converter_types , and /converter-options currently read only request_converters. As a result, an attack with only response converters would execute correctly but appear in history as having no converters and could not be found by converter filters
| ) | ||
|
|
||
| @staticmethod | ||
| def _merge_converter_identifiers( |
There was a problem hiding this comment.
Should repeated converters within additions be preserved? These fields are documented as ordered converter pipelines, but A configured pipeline such as A → B → A executes all three steps, but this hash-based deduplication stores it as A → B , causing the identifier to describe different behavior from what ran. _get_converter_identifiers() also flattens away piece-index and data-type selectors.
Guess it depends on whether these fields are intended to represent an exact pipeline or only an aggregate set of converters? If they represent the pipeline, repeated steps and configuration semantics probably need to be preserved.
| Update attack recency and converter tracking after a message is added. | ||
|
|
||
| Bumps the attack's ``timestamp`` column (the single indexed recency key) so the edited | ||
| conversation re-floats to the top of the History view. |
There was a problem hiding this comment.
nit: doc strings for variables?
| request_converter_ids = self._get_converter_identifiers(configurations=request_converter_configurations) | ||
| response_converter_ids = self._get_converter_identifiers(configurations=response_converter_configurations) | ||
| if request_converter_ids or response_converter_ids: | ||
| aid = ar.get_attack_strategy_identifier() |
There was a problem hiding this comment.
nit: rename to something that stands out more or explains what the variable is more? attack_strategy_identifier is long but could be helpful to have a more descriptive variable name
| request_converter_configurations=[ConverterConfigurationRequest(converter_ids=["c-1", "c-2"])], | ||
| ) | ||
|
|
||
| with ( |
There was a problem hiding this comment.
nit: we repeat alot of setup, could we make a helper function and move the repeated setup into 1 spot? ie make a ```async def _send_message_and_get_update_fields(
*,
service: AttackService,
request: AddMessageRequest,
attack_result: AttackResult,
) -> dict[str, Any]:
# Shared mocks and invocation live here.
await service.add_message_async(...)
return update_fields
Then each test focuses on its unique behavior:
async def test_add_message_tracks_response_converters(...):
update_fields = await _send_message_and_get_update_fields(
service=service,
request=request_with_response_converters,
attack_result=attack_result,
)
assert ...
2 participants
Description
Adds REST support for ordered, registry-backed request and response converter configurations without changing the
PromptNormalizercontract.converter_idsrequest pipeline for compatibility and emits a deprecation warning for removal in PyRIT 1.3.0.send=False.Tests and Documentation
UV_NO_SYNC=1 uv run --no-sync python -m pytest tests/unit/backend/test_attack_service.py tests/unit/backend/test_api_routes.py -q(206 passed).UV_NO_SYNC=1 uv run --no-sync pre-commit run --all-files(all hooks passed).