Repository navigation
FEAT: add ScenarioPresetRegistry owning preset storage and validation - #3057
hannahwestra25 merged 13 commits into
Conversation
6c86c4b to
ebeee48
Compare
Richard Lundeen (richlundeen)
left a comment
There was a problem hiding this comment.
Review summary
Thanks for pulling this out of #2931 — reviewing the storage and lock behaviour on its own was much easier.
What's working
The storage layer fixes the defects from #2931 and I think it's ready as-is:
_replace_filestages to a temp file, fsyncs, thenos.replace, so a failed write keeps the old preset.- The lock is an OS advisory lock released by the kernel on exit, which replaces the 60-second age heuristic that couldn't prove the first writer had died.
- Names are validated before any read, so one bad file can't stop the listing.
- Blob uses
overwrite=Falsefor create andIf-Matchfor update. custom_initializer_storage.pydrops 137 → 76 lines, so the extraction is real reuse rather than speculative.- The tests cover the hard cases: lock contention, dead-writer reclaim, partial writes, non-UTF-8 documents, blob preconditions.
I also ran it against a plain local directory to confirm nothing needs blob: save/load/list/delete, version conflicts, hand-edit detection, and the resolve guard all work, and azure.storage.blob is never imported.
Why I'm being picky about [1] and [2]
This isn't only the preset layer. It's the first durable named-configuration layer in the registry, and the intent is to persist target, scorer, and converter configurations the same way. Whatever shape lands here is what the next several registries copy, and #2931 rebases onto it immediately.
That makes two otherwise-minor things worth getting exactly right:
- [1] —
TargetRegistryandScorerRegistryare alreadyRegistrysubclasses holdinginstances. Giving them durable configs means adding storage to aRegistry, which is exactly what [1] asks for. If presets land as a bespoke class, the pattern doesn't transfer and the next registry reinvents it. - [2] — "resolve through the real path, report advisorily" is the validation model every stored config needs, and it's what keeps a config portable between deployments. A second implementation here becomes a second implementation everywhere.
The rest is ordinary and can be taken or left on its merits.
The items
I've left these as separate inline threads so we can discuss and resolve them individually.
| # | Item | Priority |
|---|---|---|
| 1 | Subclass Registry; add a registration surface |
Blocks merge |
| 2 | check_preset should use the registry's resolution |
Blocks merge |
| 3 | Blob error on a missing container | Blocks merge (small) |
| 4 | PR description fixes | Blocks merge (small) |
| 5 | Two method names that mislead | Cheap |
| 6 | Documentation | Cheap |
| 7 | Document conventions to settle now | Cheap |
| 8 | Optional version check on delete | Follow-up |
[4] PR description fixes
No code to anchor this to, so putting it here.
scenario_preset.pymoved into this PR, so the #2956 ask moved with it. Suggest adding: "Related to #2956. Prefer to merge #2956 first. Then update presets to use the same dataset-limit normalization and GUI controls, includingall." #2956 is still open, so merging this first is fine.- "
check_presetis the single validation path, replacing the service's ad-hoc_unknown_*helpers" —scenario_preset_service.pydoesn't exist onmain, so nothing was replaced; the helpers came over from #2931 largely unchanged. That line becomes true after [2].
Drafted with GitHub Copilot; reviewed and posted by me.
53188f4 to
c77303e
Compare
Prerequisite carved out from under microsoft#2931 so the durable-definition layer lands before the backend service and Registry tab UI stack on top of it. ScenarioPresetRegistry is the single owner of preset storage, validation, and version-checked resolution. It reads through to storage on every call and is deliberately not an instance cache: the version guarding every write is sha256 of the stored bytes, multi-worker and multi-replica are supported topologies, resolve_preset must compare a client-supplied expected_version against storage rather than against itself, and get_preset_version exists to reach documents that cannot be parsed at all. check_preset is the only validation path, replacing the service's ad-hoc _unknown_* helpers. It reads ScenarioMetadata off ScenarioRegistry directly rather than through the backend scenario service, so it stays synchronous and keeps pyrit.registry free of any pyrit.backend import. _delete_document reports whether the delete removed anything, taking the answer from the delete itself on both the local and blob paths, so delete reporting stays correct when two callers race to remove the same document. A parse-only startup pass reports unusable documents in the boot log without building scenario metadata, which would instantiate every registered scenario class and would raise at that point in startup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A local delete took no lock, so a delete landing inside a conditional save's read-compare-replace was reported successful and then silently undone by that replace. The blob backend already reported the same interleaving as a conflict. Take the document lock on the local delete path so both backends agree. Migrate ScenarioConfigurationResolver onto the shared technique-token grammar in pyrit/scenario/core/_technique_tokens.py so preset validation and the launch path cannot drift apart. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Blob create and update are guarded separately, so a missing container reports a storage error rather than a phantom concurrent edit. Preset documents carry a schema_version. A document written by a newer build is skipped as unusable rather than reinterpreted as the current shape, which unknown- field rejection alone does not cover. validate_stored_presets becomes scan_stored_presets. It parses, logs and counts, and retains nothing; "load" would imply the retention this registry does not do. Trims reviewer-facing argument out of the docstrings, records why this does not build on StorageIO, documents the local lock files, and notes that preset documents name a registered component rather than pinning a class. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Addresses review items 1b, 1c, 2a, 2b and 8. Technique token resolution moves out of the backend resolver and into `pyrit.scenario.core.technique_tokens`, beside the grammar it reads. It collects issues instead of raising, so the launch path can stop at the first bad token while the preset check reports everything at once. Both now resolve through the same code. `ScenarioMetadata` carries the scenario's technique catalog so a caller can resolve a token without instantiating the scenario. Preset parameter validation calls `resolve_declared_params` rather than comparing names, so a declared parameter carrying a value the scenario cannot coerce is reported instead of passing and failing at launch. `ScenarioPresetRegistry` subclasses `Registry` and drops its hand-rolled singleton. `ensure_preset` lets code contribute a preset without overwriting one the operator has since edited. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Adds the registration surface from review item 1a: a preset can now be contributed in-process by code as well as stored as a document. ScenarioPresetRegistry moves to pyrit/registry/components/, holds the registered half in `instances`, and reads merge the two with a stored document winning a name collision. ScenarioPreset gains Identifiable so the instance registry can build metadata for it, and overrides get_identifier() to rebuild on every call: the inherited cache lives on the instance, so model_copy(update=...) would otherwise carry it onto the copy and a modified copy would report the original's identity. Reads return the new PresetEntry, which names the origin of the preset it carries and leaves `version` unset for a registered one, since only a stored document can be written back. save_preset refuses a name code has registered, so one name has one owner. Also trims docstrings across the preset files per review item 6, dropping reviewer-facing argument and keeping behavior, Args, Returns, Raises, and the "why" notes where the code reads wrong without them. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
save_preset refused a code-registered name with a bare ValueError, which forced callers to either string-match the message or report every other ValueError on that path as a conflict. Raise ScenarioPresetNameRegisteredError instead so callers discriminate by type. It is a standalone ValueError subclass rather than a DocumentConflictError sibling: that base is version-shaped and advises reloading, which is wrong for a condition re-reading never clears. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The functional Enum API builds a new class, but the overload ty selects is the member-lookup form, so it infers an instance. The previous directive used a mypy rule code this repo never runs, so ty did not honor it and make ty failed on tests/unit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
c688842 to
9f84553
Compare
Halve the FileDocumentStorage class docstring. The name-validation and malformed-file skipping rationale is already stated on _list_documents, and the out-of-band edit rationale on _compute_version, so the class docstring only needs the layout, bytes, and write-safety contracts. Drop the passages in get_preset_version and save_preset that described how a caller would use the method rather than what the method does. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…io-preset-registry-foundation # Conflicts: # .pyrit_conf_example # pyrit/scenario/core/__init__.py # pyrit/setup/configuration_loader.py # tests/unit/backend/test_scenario_run_service.py
… preset branch Resolves the ScenarioPresetRegistry move to pyrit/registry/components/ and reconciles the preset form layer with microsoft#3143's scenario dataset controls. ScenarioDetail.tsx takes main's version wholesale: microsoft#3143 added supportsParameter gating and a max_concrete_techniques cap to the technique selector, so re-applying this branch's earlier extraction would have reverted that behaviour. Deletes pyrit/registry/scenario_preset_registry.py, an orphaned leftover of PR A's move that nothing imported. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…squash-merged main squash-merged microsoft#3057 as 822cbd4, a single-parent commit. This branch already carried PR A's original, unsquashed commits (9f84553 is an ancestor), so git saw two independent lineages producing the same content and raised add/add conflicts across every file microsoft#3057 touched. Resolution, and the evidence for it: - The four add/add files -- scenario_preset.py and the three preset test modules -- are byte-identical between 9f84553 and 822cbd4, so main's side contributed nothing this branch did not already have. Took ours, which is PR A's exact content plus the dataset-request migration. Verified afterwards that all five preset files kept their pre-merge blob hashes. - .pyrit_conf_example, scenario/core/__init__.py, configuration_loader.py and test_scenario_run_service.py conflicted only where main added target_upload_directory and IncompatibleTechniquePolicy. Our side was empty at each hunk, so main's additions were taken. Resolved by hand rather than with --theirs, because this branch had non-conflicting changes elsewhere in all three Python files that --theirs would have discarded. - The merge also silently duplicated the technique_tokens import block in scenario/core/__init__.py -- one copy from each lineage, in different positions. Removed the mis-sorted copy; the file now matches main exactly. Verification: of the 25 files microsoft#3057 touched, only the four carrying the dataset migration differ from origin/main. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Description
Prerequisite extracted from #2931, which rebases onto it. Durable layer only — no service, routes, or UI.
pyrit.registry.components.ScenarioPresetRegistryowns preset storage, validation, and version-checked resolution. A preset is a stored JSON document or one registered by code; reads merge both, stored wins, andPresetEntry.originreports which arrived. Saving over a registered name raisesScenarioPresetNameRegisteredError, mapped by type.It reads through on every call and is not a cache: versions are
sha256(stored_bytes), multi-worker is supported, and a cached version checked against itself would launch a configuration nobody confirmed.One technique-token resolver (
pyrit.scenario.core.technique_tokens) collects issues rather than raising, socheck_presetreports all at once. Parameters go throughresolve_declared_params.scan_stored_presetsis parse-only at boot: building scenario metadata instantiates every scenario class.Addresses asks 1–4 of the #2931 review.
#2956 and #3143 have since merged, so
max_dataset_sizeis now a deprecated alias formax_total(removed in 1.4.0).ScenarioPresetstill persists the pre-#2956 shape and cannot expressall; moving it tomax_per_dataset/max_totalis a follow-up, since #2931 builds routes and UI on the current field names.Tests and Documentation
scenario_presets_sourceand its lock files documented inpyrit_conf.md.