Repository navigation
FIX: Accept only supported external inputs when building components - #2959
varunj-msft wants to merge 10 commits into
Conversation
The registry coerced string values and enums, but any other JSON value was passed to the constructor unchecked. A list for an integer or an object for a float either built a component with invalid state or failed inside the constructor as a server error. resolve_constructor_args now checks JSON values (null, strings, numbers, booleans, arrays, and objects) against the declared parameter type with strict pydantic JSON validation before construction, and raises ValueError on a mismatch. Abstract collection types such as Collection, Sequence, and Iterable are checked as arrays, and protocol types require a value that provides the protocol's members. Values that would need conversion into objects such as paths, enums, models, or other classes are rejected too, since the constructor would receive raw JSON. Lists of enum or literal choices are converted through Parameter.coerce_value like a single enum, so they accept the choices the catalog advertises. Arrays become tuples or sets where the type declares them; other values are passed through as received, so integers still satisfy floats. Live Python objects (including list and dict subclasses) and annotations pydantic cannot resolve (such as forward references) are left to the constructor, so in-process callers are unchanged. Registry reference parameters such as converter_target and targets only accept registry names or instances, but a JSON number, boolean, or object was passed to the constructor as-is. These now raise ValueError too. WordDocConverter and A2ATarget imported some annotation types only for type checking, so the registry could not see existing_docx as a Path parameter (skipping the upload handling for it) or check prompt_template and auth_token. Those types are now imported at runtime.
|
I agree with the goal of returning clear validation errors, but I do not think the registry should derive a general JSON contract from arbitrary Python annotations. 1. Define the registry parameter contract
The registry should support only a small, explicit set of input types:
Unsupported or unresolved parameter types should not become implicit JSON APIs. The registry should reject them rather than infer conversion rules or pass JSON through silently. Constructors should remain responsible for component-specific semantic validation and should fail during initialization when a supplied value is invalid. 2. Fix the stale-registration bug
The bug is that the backend registers the object before it finishes converting the object into the API response. If identifier generation, capability access, recursive mapping, or DTO validation fails, the create request fails but the object remains registered. The converter flow can also retain uploaded artifacts in this case. The complete create operation should be atomic. We should either construct and validate/map the response before registration, then register only after all steps succeed, or roll back registration and other owned resources if any post-registration step fails. 3. Make registry input support explicit On Registry metadata should distinguish between parameters that can be supplied through JSON-backed registry surfaces, parameters that accept only existing in-process objects, and parameters that cannot be configured through the registry. Unsupported parameters should not be returned as ordinary configurable parameters. The metadata should either omit them or expose an explicit capability such as The resolver must enforce the same contract and reject JSON supplied for unsupported parameters, even when a client bypasses the GUI. 4. Make the GUI honor the registry contract The GUI should not decide whether a parameter is supported by parsing or guessing from Today, the target dialog uses bespoke forms and does not expose parameters such as Once the registry exposes an explicit parameter capability, every GUI surface should use it consistently:
The backend must still reject unsupported input because GUI restrictions are not a correctness boundary. I suggest replacing the broad annotation projection in this PR with this explicit registry contract, keeping component-specific validation in constructors, and fixing the create/map/register lifecycle independently. This prevents Python annotations from becoming an implicit public JSON schema while keeping the registry and GUI behavior predictable. |
Each derived Parameter now reports an input_kind: scalar, collection, reference, structured, in_process_only, or unsupported. The resolver converts JSON only for scalar and collection parameters, using explicit rules instead of a pydantic projection of the annotation, and rejects JSON for in_process_only and unsupported parameters. Live Python objects still pass through to constructors. Registry metadata and the converter and target catalogs report whether a class is constructible, meaning every required parameter can be supplied as JSON. framework.md documents the contract.
Agreed on all four. I replaced the projection with an explicit contract: each registry parameter now reports an input_kind (scalar, collection, reference, structured, in_process_only, unsupported). JSON is only converted for scalar and collection params, with explicit rules, and JSON for in_process_only or unsupported params is a 400. Live objects still pass through, and constructors keep their own validation. The converter and target catalogs now include constructible, and framework.md documents the contract. The create/map/register fix is in its own PR. I'll do the GUI side after #2846 lands since it rewrites the target dialog. Heads-up for that: the converter dialog sends top-level list params as text, so those now get a 400 instead of quietly passing a string to the constructor. |
|
So... you did basically what I asked, but it's not what I actually intended. It broadended a lot of things and I think left it in a worse state. I suggest separating four responsibilities:
For this PR, please narrow the change to enforcing the supported external-input boundary. Remove the general recursive JSON conversion and annotation-classification machinery, including implicit support for nested Please also reconsider whether Basically, in this PR I'd only make the following changes
|
…idate-Component-Parameters
Component validation stays in constructors. The registry only enforces which inputs external callers may supply. Removes the input_kind classification, the recursive JSON conversion, the value-shape caller detection, and the constructible and wire metadata. External callers now go through an explicit path (Registry.create_instance_from_external_input, or create_named_instance with external_input=True). It accepts only parameters with Parameter.is_external_input: scalars, lists of non-path scalars, unions of str with those or with callables (such as api_key: str | Callable), registry references given by name, and declared structured inputs. Unknown names are rejected. In-process callers can still pass any Python object. The converter, target, and scenario catalogs list only those inputs. Scenario runs and estimates apply the same check to scenario_params before the server merges its own values. framework.md states the rule in one sentence.
Thanks, that's fair, I over-built it a bit. I've cut it back to just the boundary: one is_external_input check (scalars, lists of scalars, registry names, declared structured inputs, plus str unions like api_key: str | Callable since the GUI sends a string). REST now builds through create_instance_from_external_input, so unknown or unsupported params and non-name references get a 400 before construction, and in-process create_instance is back to how it is on main. Values are left to constructors. The catalogs only list those inputs, input_kind/wire_input_kind/is_json_configurable/constructible are gone, and framework.md has your sentence. Since the CLI only creates scenarios, I applied the same check to scenario runs and estimates (it was offering --dataset-config as a flag). The create/map/register fix stays in #2964. |
varunj-msft
changed the title
|
Does this filtering affect only the external configuration catalogs? Please keep all parameters and their types in the internal registry metadata, including opaque and non-external parameters. REST and CLI configuration catalogs should expose only supported external inputs, but Python discovery and documentation still need the full component contract. |
… params in the worker - Treat flat list/Collection/Sequence of non-path scalars, and unions with such an alternative and no path alternative, as external inputs, so font_size=24, n_seconds=8, and word lists work through the API. - Check scenario_params in the preparation worker after resolving the scenario class, so a cold registry is never discovered on the API event loop. - Build REST-created scorers through the external path and list only the scorer parameters external callers may set. - Add independent catalog and creation tests, real constructor-failure cleanup coverage, and file-collection rejection coverage.
Yes, only the external catalogs are filtered. The REST catalogs drop non-external inputs and REST creation rejects them, but the registry metadata keeps every parameter and its type. There are tests for that on both targets and converters. For reference, the GUI reads the type catalogs and the CLI reads the scenario catalog. One thing to flag: #2990's scorer endpoints landed while this was open, so I put them behind the same boundary. That takes 18 of the 58 types out of Also, the GUI still shows the union and |
| @@ -277,6 +278,36 @@ def is_string_coercible(self) -> bool: | |||
| return False | |||
| return _is_scalar_param_type(_unwrap_optional(self.param_type)) | |||
|
|
|||
There was a problem hiding this comment.
The allowlist now accepts these inputs, but the catalog still describes their full Python annotations. The GUI therefore cannot configure several parameters that REST now accepts:
- SATA's
Collection[str]parameters serialize withis_list=False, so the converter form disables them. font_size: int | tuple[int, int]remains disabled by the converter form's union-type check.OpenAIVideoTarget.n_secondsis excluded by the target form's type check.
Please make the external catalog describe the supported external form, while keeping the complete Python contract in internal registry metadata. Reuse the existing descriptors: supported flat collections should use the existing list representation, including element choices for enums. Mixed unions should expose the supported external alternatives, not require clients to interpret opaque Python alternatives.
Main already represents list[SomeStringEnum] as list[str] with is_list=True and enum-value choices. Please keep the new collection support consistent with that contract.
Please also add coverage for serialized catalog metadata and the actual form submission path. The current creation tests send correctly typed values directly, so they cannot detect this mismatch. Check that the forms can supply font_size=24, n_seconds=8, and SATA word lists with the correct JSON shapes.
This should remain a small external-input projection, not recursive JSON conversion or component validation in the registry.
Richard Lundeen (richlundeen)
left a comment
Richard Lundeen (richlundeen)
left a comment
There was a problem hiding this comment.
One comment I think should still be fixed but it's looking good!
This branch has not been deployed
2 participants
Description
REST callers could set any constructor parameter, including ones that only make sense as Python objects (
custom_configuration,jailbreak_template, callables). The JSON value reached the constructor and usually failed with a 500. The CLI had the same problem for scenarios: it turned every catalog parameter into a flag, so--dataset-config xsent a raw string into an opaque scenario input and REST queued the run.This PR only enforces the external-input boundary. Component validation stays in constructors.
Allowlist:
Parameter.is_external_input. External callers may set:StructuredParameterValue);str,int,float,bool,Path,Path | str,Literal,Enum, optionallyNone;list,Collection, orSequenceof non-path scalars, e.g.SATAMaskingConverter.stopwords: Collection[str];api_key: str | Callable[...],font_size: int | tuple[int, int],n_seconds: int | Literal["4", "8", "12"].Everything else (objects, dicts, tuples, sets, nested collections,
Any, unresolved annotations) is in-process only. Nothing is converted; allowed values reach the constructor as before.Explicit path:
Registry.create_instance_from_external_input(name, params=...),create_named_instance(..., external_input=True), andresolve_constructor_args(..., external_input=True). REST converter, target, and scorer creation use it.create_instanceis unchanged, so in-process callers can still pass Python objects (e.g. the converter initializer'sTextJailBreak).Rejected before construction (400): external input that names an unknown parameter, a parameter outside the allowlist, or a registry reference given as anything but a name.
Catalogs:
/api/converters/types,/api/targets/types, and/api/scorers/typeslist only external parameters, and omit types with a required non-external parameter. Registry metadata keeps every parameter.GridCompositeConverter,SelectiveTextConverter,TextJailbreakConverter,TokenBijectionConverter;PlaywrightTarget,PlaywrightCopilotTarget,WebsocketTarget;PromptShieldTarget, an observation source, and scale, rubric, classifier, and policy models), andRegexScorer(dict) andPackageHallucinationScorer(set) fall under the dict/set rule. This part can be split out if preferred.Scenarios:
/api/scenarios/cataloglists only external parameters, so the CLI no longer offers--dataset-config,--scenario-techniques, or--technique-converters. A run checksscenario_paramsin the existing preparation worker, right after the scenario class resolves and before initializers run, so a cold registry is never discovered on the event loop; resumes go through the same worker. Estimates apply the same check. The dedicatedtechniques,dataset_names, andlabelsfields are unchanged.Runtime imports:
WordDocConverterandA2ATargetimport annotation types at runtime, soexisting_docxresolves to an upload path andauth_tokento a string input.Docs:
framework.mdgets the one-line rule: "The registry accepts only explicitly supported external inputs, permits opaque Python objects only for in-process callers, and leaves component validation to constructors."doc/gui/0_gui.mddescribes what the/typesendpoints return and drops a stale note about converter and target/catalogroutes that no longer exist.Follow-ups, not in this PR:
OpenAIChatTarget(temperature={"a": 1})andAddImageTextConverter(font_size=[12, 14])raise aTypeError(500), as on main. Fixing those belongs in each constructor.Collectionparameters (font_size,stopwords,n_seconds) still render as plain text fields, as on main. Typed controls, and dropping the hard-codedCOMMON_SCENARIO_PARAMETER_NAMESlist in favor of the filtered catalog, are separate work.SeedPrompttemplates. A component that needs one can declare a structured input.Coordination:
GridCompositeConverter.innocuous_images. Both PRs changetarget_service.pyandconverter_service.py; the second to merge keeps this PR'screate_instance_from_external_inputand catalog filtering together with FIX: Keep backend media and upload inputs inside managed storage #2958's server-resource checks.Tests and Documentation
tests/unit/models/test_parameter.py:is_external_inputfor each allowed and rejected form, including flat collections, scalar alternatives, and path-mentioning unions.tests/unit/registry/test_resolution.pyandtest_converter_registry.py: external and in-process paths, unknown and non-external parameters, name-only references, and real components.tests/unit/backend/test_converter_service.py: fixed per-component catalog expectations; registry metadata keeps every parameter; creation withAddImageTextConverter(font_size=24)and SATA word lists;GridCompositeConverter.innocuous_imagesrejected for a local path and a URL with nothing registered; a realWordDocConverterconstructor failure (placeholder=""with an uploadedexisting_docx) removes the upload and registers nothing.tests/unit/backend/test_target_service.py: fixed catalog expectations, registry metadata, andOpenAIVideoTarget(n_seconds=8).tests/unit/backend/test_scenario_run_service.py: the check runs on the real preparation worker, off the event loop; it fails against the old placement.tests/unit/backend/test_scorer_service.pyandtest_scenario_service.py: scorer and scenario catalogs and creation.font_sizes, SATA's two word lists, andn_seconds.python -m pytest tests/unit/registry tests/unit/models tests/unit/backend tests/unit/cli tests/unit/converter tests/unit/setup tests/unit/scenario tests/unit/score -n 4: 12324 passed, 5 skipped. The 21 failures are environmental: twotest_import_guards.pychecks hit a 30 s subprocess timeout, onetest_reinitialization.pyordering flake, five OTel scorer tests that fail on main too, and 13 local classifier tests that crashed a parallel worker and pass when run alone.ruff format/ruff check: passed.ty check pyrit/: same diagnostics as main.0c0c7f042: all checks passed, including unit tests on ubuntu, windows, and macOS across Python 3.11–3.14..envand the default initializers: 19/19 checks.font_size=24, the SATA word lists, andn_seconds=8are listed and created through REST;GridCompositeConverteris omitted and its file collection rejected; aPDFConvertercolor tuple is still rejected; aWordDocConverterconstructor failure returns 400 and leaves the name free; scenario runs and estimates rejectdataset_config; the scorer catalog omits the composite scorer that needs a callable, and REST rejects a callable parameter./api/scorers/typeslists 40 types, and creatingRegexScorerreturns a 400 that says to passpatternsfrom Python.