MAINT: Refactor converter construction into ConverterClassRegistry#1963
MAINT: Refactor converter construction into ConverterClassRegistry#1963rlundeen2 wants to merge 5 commits into
Conversation
Move converter class discovery, parameter introspection, string->typed param coercion, and instantiation out of the backend ConverterService into a new core ConverterClassRegistry so agents/attack strategies can build converters on the fly without depending on pyrit.backend. - Add ConverterClassRegistry with ConverterMetadata / ConverterParameterMetadata (requires_llm on params, derived is_llm_based) and a coercing create_instance. - ConverterService now delegates construction to the registry and keeps only presentation/transport concerns (DTO mapping, data-URI persistence, API-ref resolution). - Move catalog-visibility (hiding base/helper converters like SelectiveTextConverter) to the frontend, since it is purely a display decision. - Route scenario construction through ScenarioRegistry.create_instance. - Retarget tests so registry behavior is covered in the registry test module. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ScenarioResult.completion_time is datetime | None (None while a run is in progress), but ScenarioRunSummary.updated_at requires datetime. Fall back to creation_time so an in-progress run reports its creation as the last status-change time. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
|
||
| // Converter classes the backend can build but that aren't useful to offer in the | ||
| // picker (base/helper classes). | ||
| const HIDDEN_CONVERTER_TYPES = new Set<string>(['SelectiveTextConverter']) |
There was a problem hiding this comment.
Is this something we want to eventually offer in the gui? also right now it seems like this is hard-coded here and in the tests. If this is a set we expect to expand or change (probably not really the case), there could be an attribute like gui_visible in the metadata.
There was a problem hiding this comment.
Rich's response (copilot generated): Good idea, but we're deliberately keeping visibility out of the registry metadata. The registry/catalog intentionally surfaces every constructible converter so agents and attack strategies can build anything (e.g. SelectiveTextConverter is hidden from the picker but must stay discoverable/buildable). Which entries to show is a presentation concern owned by the frontend, so the hide-list stays here in ConverterPanel.tsx — and there's a test (test_metadata_has_no_catalog_visible_field) that enforces metadata staying presentation-free. If this list grows or needs to vary per surface, we'd add a presentation-layer config rather than a gui_visible field on the converter class metadata.
| self._ensure_discovered() | ||
| entry = self._class_entries.get(name) | ||
| if entry is None: | ||
| raise ValueError( |
There was a problem hiding this comment.
NIT: KeyError to match base class registry
There was a problem hiding this comment.
Rich approved fix: Done. The not-registered path now raises KeyError to match BaseClassRegistry.get_class. Both BuildableRegistry.create_instance and a new get_class override raise KeyError for an unknown name; resolution failures (unknown param, bad coercion, unresolvable registry reference) stay ValueError. The converter route translates the KeyError into a 400 at the service boundary so the API behavior is unchanged.
| is_llm_based=any(p.requires_llm for p in parameters), | ||
| ) | ||
|
|
||
| def get_converter_class(self, *, converter_type: str) -> type[PromptConverter]: |
There was a problem hiding this comment.
why is this needed? doesn't the parent class's get_class do the same thing?
There was a problem hiding this comment.
Rich approved fix: Agreed, it was redundant. Removed get_converter_class; callers now use the inherited get_class. I also overrode get_class in BuildableRegistry so its "not found" message lists the class catalog (get_class_names) rather than the instance container that a ContainerRegistry exposes via get_names.
| coerced[name] = int(value) | ||
| elif annotation is float: | ||
| coerced[name] = float(value) | ||
| elif annotation is bool: |
There was a problem hiding this comment.
do we also want to check for Literal? I'm imagining if an agent hallucinates a Literal value, we might want to catch it here. Below is what GHCP suggests for a check like this:
elif get_origin(annotation) is Literal:
allowed = get_args(annotation)
if value not in [str(a) for a in allowed]:
raise ValueError(
f"Parameter '{name}' expects one of {allowed}, got {value!r}"
)
# Coerce to the actual Literal member type (usually str, but could be int)
for member in allowed:
if str(member) == value:
coerced[name] = member
break
There was a problem hiding this comment.
Rich approved fix: Added. The coercion logic now lives in pyrit/registry/resolution.py, and coerce_string_to_annotation validates Literal values against the allowed members, raising ValueError for anything else (so a hallucinated value is caught instead of silently passed through). It returns the matching member in its real type (e.g. an int literal comes back as int). Added unit tests for the valid / invalid / int-member cases.
Move converter construction out of the backend service into the registry layer. Introduce a shared construction primitive (resolution.py) and a BuildableRegistry -> ContainerRegistry layering, then merge the separate ConverterClassRegistry into a single ConverterRegistry that both holds pre-configured instances and builds new ones from a name plus arguments. The backend ConverterService becomes a thin consumer that routes creation through the registry and retains only presentation/preview concerns. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Raise KeyError (not ValueError) for unknown names in BuildableRegistry to match BaseClassRegistry; resolution failures stay ValueError. Converter route translates the KeyError to a 400 at the service boundary. - Remove redundant get_converter_class; callers use the inherited get_class. Override get_class in BuildableRegistry so the "not found" message lists the class catalog rather than the instance container. - Validate Literal values in coerce_string_to_annotation (raise on a value outside the allowed members) and return the matching member in its real type. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2 participants
<Not in draft exactly, but going to scheduel a design>
Construction logic previously lived in the backend service, so attack strategies and on-the-fly agents couldn't build a converter without reaching into backend code. Making "build by name" a registry capability is the keystone for agent/config-driven component assembly — including LLM converters, whose converter_target is resolved by name from the TargetRegistry.
This PR converter construction out of the backend service and into the registry layer, so building a converter by name is a reusable, backend-free capability.
Phase 1 of this plan to unify the registry: https://gist.github.com/rlundeen2/f7960f7e8973fbb705b1b4bb48d8cdb2