Repository navigation
[BREAKING] FIX: Fail a backend scan when its default chat target isn't registered - #4127
varunj-msft wants to merge 1 commit into
Conversation
…ad of falling back to one built from environment variables
There was a problem hiding this comment.
🟡 Changes recommended
Valid scans with explicit or registered non-chat objective scorers are rejected because introspection and scorer-resolution paths unnecessarily require an unused objective_scorer_chat target.
2 open issues
What changed in this PR
This PR makes backend scans fail when a required default chat target is not registered, rather than silently using an environment-configured OpenAI target. It preserves fallback behavior for notebooks and scripts outside the strict scope.
Changes:
- Adds and exports a scoped requirement for registered default targets.
- Applies strict resolution to backend launch checks, preparation, and execution.
- Adds regression tests and documents the backend configuration requirement.
| File | Description |
|---|---|
tests/unit/scenario/core/test_scenario_target_defaults.py |
Tests strict resolution, fallback restoration, and target overrides. |
tests/unit/backend/test_scenario_run_service.py |
Tests launch failure when a required default is missing. |
pyrit/scenario/core/scenario_target_defaults.py |
Adds scoped enforcement of registered defaults. |
pyrit/scenario/core/__init__.py |
Exports the new context manager. |
pyrit/backend/services/scenario_run_service.py |
Enforces registered defaults throughout scan execution. |
pyrit/backend/README.md |
Documents missing-default failures and registration requirements. |
Review effort: Balanced
| ) | ||
| 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(): |
| if _registered_defaults_required.get(): | ||
| raise ValueError( | ||
| f"No target is registered as '{preferred_target_key}', so there's no default to use. Register one " | ||
| "under that name, for example in a startup script, or choose a target explicitly." | ||
| ) |
| ) | ||
| 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(): |
There was a problem hiding this comment.
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 OPENAI_CHAT_* fallback. One thing to settle as a team: do we want the library-side silent fallback to survive long-term, or should it be deprecated separately? No change needed in this PR either way.
| finally: | ||
| await service.shutdown_async() | ||
|
|
||
| async def test_a_scan_that_needs_an_unregistered_default_target_fails_to_start_async( |
There was a problem hiding this comment.
Non-blocking: This covers the prepare-time scope nicely. Could you add one for the rerun path — _validate_rerun_references resolving a default that's gone missing (e.g., stored references contain an adversarial_target_hash but the registry entry was removed before the rerun)? That's the one wrapped site no test currently reaches.
| ) | ||
| target = target if target is not None else get_default_adversarial_target() | ||
| if target is None: | ||
| with require_registered_default_targets(): |
There was a problem hiding this comment.
Nit, optional: this scope is already covered by the one entered in _prepare_run_async (line 981), since _capture_launch_references is only called from _initialize_scenario_async inside that scope and asyncio.to_thread propagates contextvars. Harmless as defensive duplication — fine to keep.
|
Closing this one: #3794 on |
Description
When a backend scan needs a default adversarial or scorer chat target that the server didn't register as
adversarial_chatorobjective_scorer_chat, it now fails with that reason instead of quietly running against anOpenAIChatTargetbuilt from theOPENAI_CHAT_*environment variables. Rich suggested this in the review of #3908 (point 3) as its own small fix on main: a misspelled name in a startup script should stop a scan, not swap in a different target.require_registered_default_targets()inpyrit.scenario.coreis a scope, likeoverride_default_adversarial_target. Inside it,get_default_adversarial_target()andget_default_scorer_target()raise when the registry has no entry. Outside it, the library keeps its fallback, so notebooks and scripts that rely onOPENAI_CHAT_*work as before.Breaking for the backend: a server that only sets
OPENAI_CHAT_*and relied on that fallback now needs to register the targets, for example through the target initializer'sADVERSARIAL_CHAT_*andOBJECTIVE_SCORER_CHAT_*variables, or choose an adversarial target for the scan.