MAINT: Refactor converter construction into ConverterClassRegistry#1963
Open
rlundeen2 wants to merge 7 commits into
Open
MAINT: Refactor converter construction into ConverterClassRegistry#1963rlundeen2 wants to merge 7 commits into
rlundeen2 wants to merge 7 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>
jsong468
reviewed
Jun 11, 2026
jsong468
reviewed
Jun 11, 2026
| self._ensure_discovered() | ||
| entry = self._class_entries.get(name) | ||
| if entry is None: | ||
| raise ValueError( |
Contributor
There was a problem hiding this comment.
NIT: KeyError to match base class registry
Contributor
Author
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.
jsong468
reviewed
Jun 11, 2026
jsong468
reviewed
Jun 11, 2026
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>
Replace the interim ContainerRegistry subclass with a typed .instances property backed by DefaultInstanceRegistry, move ConverterRegistry under registry.components, and add optional runtime instance-type enforcement so a non-converter cannot be registered in the converter instance container. Annotate the legacy base/retrievable instance registries as do-not-extend. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Underscore ConverterParameterMetadata to _ConverterParameterMetadata, drop it from the registry public exports, and add a transitional note pointing to the unified Parameter contract that replaces it in Phase 3. The backend converter service imports it directly from the submodule. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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