FEAT: Enforce keyword-only __init__ on PyRIT lego brick base classes#1883
Open
romanlutz wants to merge 7 commits into
Open
FEAT: Enforce keyword-only __init__ on PyRIT lego brick base classes#1883romanlutz wants to merge 7 commits into
__init__ on PyRIT lego brick base classes#1883romanlutz wants to merge 7 commits into
Conversation
…nd SeedDatasetProvider PyRIT's extension-point base classes (PromptConverter, Scorer, PromptTarget, Scenario, AttackStrategy, SeedDatasetProvider) need a consistent constructor contract so users can swap "Lego bricks" without surprise. The style guide already mandates `def __init__(self, *, ...)` (keyword-only parameters), but until now nothing enforced it. This change is the infrastructure PR (PR 1 of 5 in the audit; see session artifacts phase1_brick_inventory.md and phase2_contract_proposal.md): * Add `pyrit/common/lego_brick_contract.enforce_keyword_only_init` — a small helper that bases invoke from their own `__init_subclass__` hook to raise `TypeError` for non-conforming subclasses at class definition time. Subclasses may set `_lego_brick_legacy_init = True` to downgrade the error to a `DeprecationWarning(removed_in="0.16.0")` via the existing `print_deprecation_message` machinery — letting publicly-positional APIs migrate over one release cycle. * Wire `Scenario.__init_subclass__` (new) and extend `SeedDatasetProvider.__init_subclass__` (existing — already registers subclasses and emits the legacy `fetch_dataset` deprecation) to call the helper. Both families were already 100%-compliant per the phase 1 inventory; this PR has no behavior change for production code. * Document the runtime check + opt-out in `.github/instructions/scenarios.instructions.md` and `.github/instructions/datasets.instructions.md`. * Fix three test fixtures whose `__init__` violated the contract (mechanical `*,` insertion; all call sites already used kwargs). Subsequent PRs will extend the same helper to AttackStrategy, Scorer, PromptConverter, and PromptTarget — each opting publicly-positional violators into `_lego_brick_legacy_init = True` so the suite stays green during the deprecation window. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds an `__init_subclass__` hook on `AttackStrategy` that calls the shared `enforce_keyword_only_init` helper (introduced in the previous commit) so every attack subclass must use the keyword-only constructor shape mandated by the style guide. All 12 attack subclasses already conformed per the phase 1 inventory, so this PR has no behavior change for production code — it just locks in the existing pattern at the class-definition boundary so future contributions can't drift. Also adds `.github/instructions/attacks.instructions.md` documenting the contract, the runtime check, and the `_lego_brick_legacy_init` opt-out escape hatch (`removed_in="0.16.0"`). The new check is complementary to the existing factory-time rejection of `**kwargs` in `AttackTechniqueFactory` — the factory check catches scenarios-side wiring mistakes, the subclass check catches the `__init__` shape at class definition time. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds an `__init_subclass__` hook on `Scorer` that calls the shared `enforce_keyword_only_init` helper so every scorer subclass must use the keyword-only constructor shape mandated by the style guide. Grandfathers the one production violator surfaced by the phase 1 inventory: * `PlagiarismScorer` (`pyrit/score/float_scale/plagiarism_scorer.py`) accepts `reference_text` positionally as part of its public API. Opting into `_lego_brick_legacy_init = True` keeps existing callers working but emits a `DeprecationWarning(removed_in="0.16.0")` at module import time. The positional shape will become keyword-only in 0.16.0 (`BREAKING CHANGE`). Fixes seven test fixture classes whose `__init__` violated the contract (`MockTextTrueFalseScorer`, `MockTextFloatScaleScorer`, `MockTrueFalseScorer`, `MockFloatScaleScorer`, `MockAudioTrueFalseScorer`, `MockScorer` in the composite scorer suite, and two anonymous fixtures in `test_scorer.py`). Where the positional callsites existed (`MockScorer(True, "...")` in the composite suite), the calls are updated to use kwargs in the same commit. Also adds `.github/instructions/scorers.instructions.md` documenting the contract, the runtime check, the `_lego_brick_legacy_init` opt-out escape hatch, and the current grandfathered list. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Wires `enforce_keyword_only_init` into `PromptConverter.__init_subclass__` so every new converter must declare its `__init__` with `*` as the first parameter after `self` (or use `*args` immediately after `self` to consume positional inputs into a back-compat shim). Violators now fail at class definition time with a `TypeError` that lists the offending parameters and points at the opt-out attribute. Twelve existing converters whose positional `__init__` is part of the public API are grandfathered with `_lego_brick_legacy_init = True` so the suite stays green; each grandfathered class emits a `DeprecationWarning` once at import time with `removed_in=0.16.0`. The grandfathered list is documented in `converters.instructions.md`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Wires `enforce_keyword_only_init` into `PromptTarget.__init_subclass__` so every new target must declare its `__init__` with `*` as the first parameter after `self` (or use `*args`/`**kwargs` immediately after `self`). Violators now fail at class definition time with a `TypeError` that lists the offending parameters and points at the opt-out attribute. The four existing public-API violators (`HTTPTarget`, `PromptShieldTarget`, `OpenAICompletionTarget`, `OpenAIImageTarget`) are grandfathered with `_lego_brick_legacy_init = True` so the suite stays green; each emits a `DeprecationWarning` once at import time with `removed_in=0.16.0`. `PromptTarget.__init__` itself still accepts positional parameters. The `__init_subclass__` hook only runs for subclasses, so the base class non-compliance is tolerated during this warn-first phase; the base signature will be reshaped to be keyword-only in 0.16.0 as a BREAKING CHANGE. A TODO comment on the base `__init__` records the plan. Creates `.github/instructions/targets.instructions.md` documenting the contract, the opt-out attribute, and the grandfathered list. Fixes the `MockPromptTarget` test fixtures in `tests/unit/mocks.py` and `tests/integration/mocks.py` so they comply with the new check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PyRIT docstrings use plain double-backtick names, not Sphinx `:func:` roles. Replace the seven `:func:...` references I introduced across the lego-brick contract PRs with the same plain-backtick style used elsewhere in the file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…brick-constructors
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
PyRIT's main extension points (
PromptConverter,Scorer,PromptTarget,Scenario,AttackStrategy,SeedDatasetProvider) are "lego bricks" that users routinely swap in and out. Today there's no enforcement of any constructor shape, so sibling classes diverge: some are kwarg-only (per the style guide), others still accept positional args, and a few are sandwiched**kwargs-passthroughs. Swapping bricks in real code feels inconsistent and surprising.This PR closes that gap with a small, shared
__init_subclass__check that every lego-brick base class invokes. Violators now fail at class-definition time with an actionableTypeError, so the contract is documented in instruction files and enforced by Python itself.Approach
pyrit/common/lego_brick_contract.pyexposesenforce_keyword_only_init(cls, *, base_name). It inspects the subclass's directly-defined__init__(skipping inherited ones) and flags any parameter afterselfwhose kind isPOSITIONAL_ONLYorPOSITIONAL_OR_KEYWORD.*args(VAR_POSITIONAL) is allowed because it already forces the rest to be keyword-only.__init_subclass__via a local import (avoids thepyrit.commoncircular-import trap):PromptConverter,Scorer,PromptTarget,Scenario,AttackStrategy,SeedDatasetProvider._lego_brick_legacy_init = True) downgrades theTypeErrorto a one-shotDeprecationWarning(removed_in="0.16.0")for the 15 existing public-API violators. New code MUST follow the contract; the opt-out attribute is intended to be removed in 0.16.0.PromptTarget.__init__itself is still positional (6 params).__init_subclass__doesn't fire on the base, so the suite stays green; a TODO comment records the 0.16.0 reshape plan.attacks.instructions.md,scorers.instructions.md,targets.instructions.md; extendedconverters.instructions.md,scenarios.instructions.md,datasets.instructions.md.Grandfathered classes (slated for 0.16.0 cleanup)
PlagiarismScorerreference_text,metric, ...)AddImageVideoConverter,AnsiAttackConverter,AsciiArtConverter,AskToDecodeConverter,DiacriticConverter,InsertPunctuationConverter,PDFConverter,QRCodeConverter,RandomCapitalLettersConverter,SearchReplaceConverter,SmugglerConverter(and its 3 children)HTTPTarget,PromptShieldTarget,OpenAICompletionTarget,OpenAIImageTargetThe two
*argsback-compat shim classes (AddImageTextConverter,AddTextImageConverter) needed no flag because*argsafterselfalready satisfies the helper.Tests and Documentation
tests/unit/common/test_lego_brick_contract.pyexercises the helper (compliant /*/*args/ inherited / positional / opt-out /print_deprecation_messageintegration).tests/unit/scenario/,tests/unit/score/,tests/unit/mocks.py, andtests/integration/mocks.pygot the mechanical*,insert so they comply with the new check. One composite-scorer test had four positional callsites updated to kwargs.tests/unit/run after every commit; the final run is 8392 passed, 120 skipped, 469 warnings. The 15 grandfathered classes emit exactly one DeprecationWarning each at import time..github/instructions/*.md; no notebook rebuilds needed. No JupyText changes.Review notes
pyrit/common/lego_brick_contract.py. The commit graph mirrors the original PR plan (one commit per base class plus a small doc cleanup commit), so reviewing commit-by-commit is the recommended path.