Skip to content

MAINT: Update Registry APIs - #2550

Open
Richard Lundeen (richlundeen) wants to merge 9 commits into
microsoft:mainfrom
richlundeen:rlundeen2-registry-api-foundation
Open

MAINT: Update Registry APIs#2550
Richard Lundeen (richlundeen) wants to merge 9 commits into
microsoft:mainfrom
richlundeen:rlundeen2-registry-api-foundation

Conversation

@richlundeen

@richlundeen Richard Lundeen (richlundeen) commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • make component registries own named construction, instance storage, lookup, duplicate and reserved-name validation, and removal
  • add registry-backed converter lifecycle and /api/converters/types APIs, plus the target registry consistency needed by later stack layers
  • support pathlib.Path constructor metadata and uploaded data-URI persistence without exposing server paths
  • keep /api/converters/catalog and /api/targets/catalog as temporary compatibility projections for the unchanged main UI; the higher chat-migration layer will remove them

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: c9eafadc-3a00-471b-b61b-68ba4d656577
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: c9eafadc-3a00-471c-a08b-28df698d8109
Type converter file inputs as Path instead of special-casing strings in the
backend: AddImageVideoConverter.video_path is now a Path, so the registry
describes it as an input file and REST treats it as an upload like every other
Path parameter.

Drop the per-parameter MIME/signature allowlist. Uploads are stored verbatim
because any file type is a legitimate payload; the generated file name means a
declared MIME type only picks an extension. Content restrictions now live only
in the media route, which serves active document types as neutralized downloads
without renaming stored files.

Mark the temporary catalog projections consistently so reviewers can see the
whole concept is deleted once the UI moves to /types.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c9eafadc-3a00-471b-b61b-68ba4d656577
@richlundeen Richard Lundeen (richlundeen) changed the title Add registry API foundation MAINT: Update Registry APIs Sep 3, 2026
Preserve registry-backed target type metadata while adopting main's strict
auth-mode validation and concurrency cleanup. Reset the technique registry in
the pre-registration test so it tests custom-first registration under strict
duplicate-name rejection.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c9eafadc-3a00-471b-b61b-68ba4d656577
Treat the media extension allowlist as an inline-rendering allowlist rather
than an access allowlist. Any stored file type, including executables and
active documents, can be downloaded as opaque bytes while only known-safe
media types receive renderable content types.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c9eafadc-3a00-471b-b61b-68ba4d656577
Keep constructor inputs in a backend-owned temporary directory, independent of result storage. Clean partial writes on failure and remove owned inputs and dependent registry entries at shutdown.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c9eafadc-3a00-471b-b61b-68ba4d656577
Add a typed registry base that builds and stores named instances. Use it for converter, target, and scorer registries while keeping the instance container storage-only.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c9eafadc-3a00-471b-b61b-68ba4d656577
Comment on lines +194 to +196
owned_paths = self._get_owned_artifact_paths(entry.metadata)
await self._remove_owned_artifacts_async(paths=owned_paths)
return self._registry.instances.unregister(converter_id) is not None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make the final unregister conditional on the original entry's identity? File cleanup yields, so two overlapping DELETE requests can race with recreation:

  1. Delete A reads the original entry and waits for cleanup.
  2. Delete B removes that entry, then a POST creates a replacement under the same name.
  3. A resumes and unregisters the replacement.

The POST succeeds, but the new converter then disappears. An atomic identity-checked removal, or serialization of deletion and recreation for each name, would prevent an older request from deleting a newer registration.

Comment on lines +279 to +280
if not replace:
self.validate_name_available(name)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The target and scorer initializers were updated for this new duplicate policy, but ConverterInitializer.initialize_async() still registers without replace=True in pyrit/setup/initializers/converters.py.

After replacing adversarial_chat and rerunning the converter initializer, registration logs Instance 'variation' already exists and keeps the original VariationConverter. Conversions therefore keep using the old target instead of the newly configured one.

Could we update the converter initializer to explicitly replace its presets too?

converter_registry.instances.register(converter, name=config.registry_name, replace=True)

TargetTypeEntry(
target_type=metadata.class_name,
parameters=[p for p in metadata.parameters if p.is_string_coercible],
parameters=[p for p in metadata.parameters if p.is_string_coercible or p.reference is not None],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we preserve whether a reference is a list when exposing it here? /api/targets/types currently describes RoundRobinTarget.targets as:

{
  "name": "targets",
  "type_name": "any",
  "is_list": false,
  "reference_type": "target"
}

However, construction requires a list such as targets=["a", "b"] and rejects a single name. The annotation is stored in reference.annotation, while the display properties inspect param_type, which is unset for references. Deserialization also drops the reference annotation.

Please derive the reference's list/scalar shape from its annotation and preserve it through serialization, so clients can distinguish single-target and multiple-target inputs without hardcoding individual classes.

Comment on lines +373 to +374
if param_types.get(name) is not Path:
continue

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This skips existing file inputs that are still typed as str, including ColloquialWordswapConverter.wordswap_path and AddImageTextConverter.img_to_add. The unchanged UI still sends data URIs from their file pickers. Those URIs now reach the constructors unchanged, get treated as filenames, and cause both creation requests to return HTTP 500.

Could we migrate the remaining file inputs to the upload contract in this PR, or preserve their previous upload handling until that migration is complete?

self,
*,
video_path: str,
video_path: Path,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This annotation makes ConverterRegistry.create_instance("AddImageVideoConverter", video_path=...) convert an Azure Blob URL through Path(...) before calling the constructor. On Windows, for example:

https://example.blob.core.windows.net/container/video.mp4

becomes:

https:\example.blob.core.windows.net\container\video.mp4

Calling str(video_path) does not restore the URL. The serializer then selects DiskStorageIO instead of Azure storage, breaking the blob-URL support mentioned in the new docstring.

Could we preserve both local paths and blob URLs when handling this parameter? Adding upload metadata should not force URL inputs through filesystem-path normalization.

@router.post(
"",
response_model=CreateConverterResponse,
response_model=ConverterInstance,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking backward-compatibility note: the /catalog routes preserve part of the old API, but we should be aware of these changes for existing callers:

  • POST /api/converters drops top-level converter_type and display_name. The type is now at identifier.class_name, so clients parsing the old CreateConverterResponse need updating even though that model remains importable.
  • Duplicate .instances.register(...) calls now raise unless replace=True is passed. Target/converter registries also reject newly reserved names such as catalog and types.
  • REST Path inputs are now upload-only, using base64 data URIs. Uploads for file parameters still typed as str are no longer decoded.
  • Changing AddImageVideoConverter.video_path from str to Path also changes registry coercion, including the handling of Azure Blob URLs.
  • Unnamed targets now receive random compat_... names instead of identifier-derived names, and converter IDs are no longer necessarily UUID strings. Clients need to use the returned names as opaque identifiers.
  • PDF, SVG, HTML, text, Markdown, and CSV media now download as application/octet-stream attachments rather than being served for inline rendering.

I'm not asking to block this PR on adding compatibility shims for all of these, but it would be worth documenting them in the migration/release notes so downstream users aren't surprised. The specific correctness regressions are covered by the separate comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants