Skip to content

FIX: restore TargetCapabilities modality combinations when reading the REST wire form - #2941

Open
Chen Yufeiyang (feiiiiii5) wants to merge 3 commits into
microsoft:mainfrom
feiiiiii5:fix/restore-target-capabilities-modalities-from-wire
Open

Chen Yufeiyang (feiiiiii5) wants to merge 3 commits into
microsoft:mainfrom
feiiiiii5:fix/restore-target-capabilities-modalities-from-wire

Conversation

@feiiiiii5

Copy link
Copy Markdown
Contributor

Description

TargetCapabilities documents itself as the REST wire snapshot of a target's capabilities, and TargetInstance — the FastAPI response model for GET /api/targets — embeds it. But the model cannot be read back. Serialization excludes the modality combination fields and emits only their flattened projections, and nothing folds those back on read:

# pyrit/models/target/target_capabilities.py:109,113
input_modalities:  frozenset[frozenset[PromptDataType]] = Field(default=_DEFAULT_TEXT_MODALITIES, exclude=True)
output_modalities: frozenset[frozenset[PromptDataType]] = Field(default=_DEFAULT_TEXT_MODALITIES, exclude=True)
# :116,133 — supported_{input,output}_modalities are @computed_field, recomputed from
# whatever input_modalities / output_modalities happen to be after validation

model_config = ConfigDict(frozen=True) leaves extra at pydantic's default "ignore", and the class has no model_validator(mode="before"). So on read-back the two combination keys are absent, the text-only default applies, and the supported_*_modalities keys that were in the payload are discarded and recomputed from that default. The model contradicts its own wire form:

caps = TargetCapabilities(output_modalities=frozenset({frozenset({"image_path"})}))
caps.model_dump()["supported_output_modalities"]      # ['image_path']   <- what goes on the wire
TargetCapabilities.model_validate_json(caps.model_dump_json())
    .supported_output_modalities                      # ['text']         <- what comes back

An image- or audio-capable target reads back as text-only; for gpt-4o all six input modality combinations collapse to ['text']. This is a live round trip: pyrit/cli/api_client.py:262 does TargetInstance.model_validate(item) on every /api/targets payload.

Parameter already solves this exact problem in the same layer — pyrit/models/parameter.py:143, _reconstruct_param_type_from_wire — citing the same consumer: "a client that deserializes the wire form (e.g. the CLI consuming the REST catalog) has those fields but no live type; this reconstructs a coercion-capable param_type from them so the round-tripped Parameter can still coerce and validate values." This adds the analogous before-validator.

One tradeoff worth naming: the wire carries the flattened union rather than the combinations, so a single combination holding that union is restored. That makes supported_*_modalities round-trip exactly — the contract the wire documents — and leaves the object self-consistent. Recovering the original combination structure would mean changing the wire format and the OpenAPI schema the CLI and UI consume; I judged the corrective fix the safer default, but say the word if you would rather have full fidelity. In-process construction is unaffected: the validator only fills the combination fields in when they are absent, so a supplied live input_modalities still wins.

Tests and Documentation

  • New TestTargetCapabilitiesWireRoundTrip in tests/unit/prompt_target/target/test_target_capabilities.py (9 tests). Six fail on ab1c6c81 and pass here. The other three pin behaviour that must not change: the default text-only round trip, in-process construction winning over a supplied flattened projection, and non-mapping payloads left to pydantic.
  • Coverage of pyrit/models/target/target_capabilities.py from that file: 100% (48/48 statements).
  • Full unit suite: 21786 passed, 241 skipped, plus one pre-existing failure — tests/unit/backend/test_scenario_run_routes.py::TestResumeScenarioRunRoute::test_resume_has_no_get_preflight. It also fails on ab1c6c81 with no changes applied (checked by stashing this diff); it is test-order pollution within tests/unit/backend, unrelated.
  • pre-commit run --files <both files> passes, including ruff format, ruff check and ty.
  • No documentation change: this corrects behaviour to match what the existing docstrings already promise.

TargetCapabilities documents itself as the REST wire snapshot of a target's
capabilities, and TargetInstance embeds it as the FastAPI response model. But
serialization excludes input_modalities/output_modalities and emits only their
flattened supported_*_modalities projections, and nothing folds those back on
read. A client that validates the payload -- as pyrit/cli/api_client.py:262 does
for every GET /api/targets -- falls through to the text-only default, so the
model contradicts its own payload:

  live   supported_output_modalities == ['image_path']
  dump   "supported_output_modalities":["image_path"]
  after  supported_output_modalities == ['text']

gpt-4o's six input modality combinations collapse to ['text'] the same way.

Add a before-validator that rebuilds the combination fields from the flattened
projections, mirroring Parameter._reconstruct_param_type_from_wire, which solves
the same wire-shape problem in the same layer. The wire carries the union rather
than the combinations, so one combination holding that union is restored --
enough to keep the flattened projection exact, which is the contract the wire
documents.
continue
flattened = data.get(flattened_key)
if isinstance(flattened, list):
combinations = [frozenset(flattened)] if flattened else []

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 bug is valid, but I would prefer to make TargetCapabilities serialization lossless rather than infer combinations from the flattened fields.

A flattened list cannot distinguish {{"text"}, {"image_path"}} (separate inputs) from {{"text", "image_path"}} (combined input). Restoring one union changes the meaning of the canonical model: it can advertise unsupported combinations and removes explicit text-only combinations. Existing requirement checks and modality routing use those distinctions.

Could we keep the immutable frozenset[frozenset[PromptDataType]] fields internally, serialize input_modalities and output_modalities as sorted lists of sorted lists, and retain the flattened supported_*_modalities computed fields for UI consumers? TargetConfiguration._capabilities_to_identifier_params() already uses that lossless, deterministic representation; the serialization itself belongs on the model, without importing the target-layer helper.

That would add fields to the wire schema without removing the fields the frontend currently reads. The round-trip tests should assert restored == caps for profiles with multiple combinations, with a nested TargetInstance/CLI deserialization case as well. The current flattened-list assertions pass even when the combinations change.

If we must retain the old wire shape, I would use an explicit summary model rather than represent unknown combinations as one declared combination.

frozenset[frozenset] fields were excluded from the wire form and rebuilt
as a single combination union, which overstates supported combinations.
Serialize them as sorted lists of sorted lists instead, keep the flattened
supported_* projections as derived computed fields, and round-trip
restored == caps in tests.
@feiiiiii5

Copy link
Copy Markdown
Contributor Author

Agreed — the wire form now carries the combinations losslessly: input_modalities/output_modalities serialize as sorted lists of sorted lists and validate back into the same frozenset[frozenset], so restored == caps. The flattened supported_*_modalities computed fields remain for UI consumers, and _capabilities_to_identifier_params() is no longer imported from the model. Round-trip tests cover multi-combination profiles, nested TargetInstance, and the CLI path; test_target_capabilities.py 59/59 green.

This branch has not been deployed

No deployments
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