Repository navigation
[BREAKING] FIX: Fail a backend scan when its default chat target isn't registered #4127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -102,7 +102,11 @@ | |
| from pyrit.registry import InitializerRegistry, ScenarioRegistry | ||
| from pyrit.registry.resolution import reject_non_external_params, resolve_declared_params | ||
| from pyrit.scenario import IncompatibleTechniquePolicy, Scenario | ||
| from pyrit.scenario.core import get_default_adversarial_target, override_default_adversarial_target | ||
| from pyrit.scenario.core import ( | ||
| get_default_adversarial_target, | ||
| override_default_adversarial_target, | ||
| require_registered_default_targets, | ||
| ) | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
@@ -405,7 +409,9 @@ def _validate_rerun_references(self, *, stored: ScenarioResult, request: RunScen | |
| target = self._configuration_resolver.resolve_adversarial_target( | ||
| target_name=request.adversarial_target_name | ||
| ) | ||
| target = target if target is not None else get_default_adversarial_target() | ||
| if target is None: | ||
| with require_registered_default_targets(): | ||
| target = get_default_adversarial_target() | ||
| if target.get_identifier().hash != original_hash: | ||
| raise ScenarioRunConflictError( | ||
| "The saved adversarial target changed. Start a new configuration explicitly." | ||
|
|
@@ -520,7 +526,9 @@ def _capture_launch_references( | |
| target = self._configuration_resolver.resolve_adversarial_target( | ||
| target_name=request.adversarial_target_name | ||
| ) | ||
| target = target if target is not None else get_default_adversarial_target() | ||
| if target is None: | ||
| with require_registered_default_targets(): | ||
| target = get_default_adversarial_target() | ||
| references["adversarial_target_hash"] = target.get_identifier().hash | ||
| return references | ||
|
|
||
|
|
@@ -969,7 +977,8 @@ async def _prepare_run_async( | |
| adversarial_target = self._configuration_resolver.resolve_adversarial_target( | ||
| target_name=request.adversarial_target_name | ||
| ) | ||
| with override_default_adversarial_target(adversarial_target): | ||
| # The server's startup scripts register its defaults, so a missing one is an error rather than a fallback. | ||
| with override_default_adversarial_target(adversarial_target), require_registered_default_targets(): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking, discussion: The scoped opt-in design is a nice conservative reading of point 3 in the #3908 review — the backend fails fast while notebooks keep the |
||
| init_kwargs = await run_legacy_sync_async( | ||
| self._configuration_resolver.resolve_configuration, | ||
| scenario_name=request.scenario_name, | ||
|
|
@@ -1759,7 +1768,7 @@ async def _execute_run_async(self, *, scenario_result_id: str) -> None: | |
| handoff_ready = True | ||
|
|
||
| try: | ||
| with override_default_adversarial_target(active.adversarial_target): | ||
| with override_default_adversarial_target(active.adversarial_target), require_registered_default_targets(): | ||
| await active.scenario.run_async() | ||
|
|
||
| except asyncio.CancelledError: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -949,6 +949,28 @@ async def fail_async(*args: object, **kwargs: object) -> None: | |
| finally: | ||
| await service.shutdown_async() | ||
|
|
||
| async def test_a_scan_that_needs_an_unregistered_default_target_fails_to_start_async( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking: This covers the prepare-time scope nicely. Could you add one for the rerun path — |
||
| self, mock_all_registries: dict[str, Any] | ||
| ) -> None: | ||
| mock_all_registries["target_registry"].instances.get.side_effect = {"my_target": MockPromptTarget()}.get | ||
| service = ScenarioRunService() | ||
|
|
||
| async def initialize_async(*args: object, **kwargs: object) -> Any: | ||
| get_default_adversarial_target() | ||
| return mock_all_registries["scenario_instance"] | ||
|
|
||
| mock_all_registries["scenario_registry"].create_and_initialize_async.side_effect = initialize_async | ||
| try: | ||
| with ( | ||
| patch("pyrit.scenario.core.scenario_target_defaults.OpenAIChatTarget") as fallback, | ||
| pytest.raises(ValueError, match="No target is registered as 'adversarial_chat'"), | ||
| ): | ||
| await service.start_run_async(request=_make_request()) | ||
| fallback.assert_not_called() | ||
| assert not service._active_tasks | ||
| finally: | ||
| await service.shutdown_async() | ||
|
|
||
| async def test_cancelled_start_keeps_scope_until_preparation_finishes_async( | ||
| self, mock_all_registries: dict[str, Any] | ||
| ) -> None: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, optional: this scope is already covered by the one entered in
_prepare_run_async(line 981), since_capture_launch_referencesis only called from_initialize_scenario_asyncinside that scope andasyncio.to_threadpropagates contextvars. Harmless as defensive duplication — fine to keep.