Add Scenario progress foundation - #2372
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5d02c2d5-b499-4f78-a04d-03bffa750817
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| is_chat_target = target.configuration.includes(capability=CapabilityName.EDITABLE_HISTORY) | ||
| if not is_chat_target: | ||
| config = prepended_conversation_config or PrependedConversationConfig() | ||
| if request_converters: |
There was a problem hiding this comment.
This overlaps with #2359 rather than only depending on it. That PR applies request converters to configured prepended roles for non-chat targets, while this hunk rejects role-scoped conversion for those targets. I think #2359 should merge first; then this PR can rebase and remove the overlapping temporary behavior here.
| chat during execution. ``None`` auto-derives from the attack | ||
| class constructor signature and seed-technique shape. | ||
| Authors can override the derivation explicitly. | ||
| supports_request_converter_composition: Whether callers may safely |
There was a problem hiding this comment.
I think this name is confusing.
supports_additional_request_converters
| When ``None`` (default), no attribution is applied. The same | ||
| attribution is shared across all tasks; per-task identity is | ||
| reconstructed from the row's own ``objective_sha256``. | ||
| attributions: Optional per-seed-group attribution. Must match |
There was a problem hiding this comment.
Should we just only have "attributions"; For this PR maybe just use attributions and normalize it immediately. Long term I think we could design it better but I don't think it's worth it here.
| ) | ||
|
|
||
|
|
||
| class ScenarioProgressKeysetCursor(NamedTuple): |
There was a problem hiding this comment.
Could we rename the existing AttackResultsKeysetCursor to AttackResultKeysetCursor and reuse it here? Both types identify the last attack-result row using the same (timestamp, attack_result_id) key.
| return value | ||
|
|
||
|
|
||
| class ScenarioRunSizeEstimateStatus(str, Enum): |
There was a problem hiding this comment.
I think this may be more complexity than we need. This is already an estimate, so callers probably do not need an Exact status; None can represent an unavailable count. max_dataset_size is optional and several scenarios do not set it, so it can help bound some estimates but cannot be the general calculation. Could we use a nullable estimated count plus a note for caveats instead?
There was a problem hiding this comment.
ScenarioRunSizeFactor duplicates arithmetic already represented by ScenarioRunSizeComponent.count: callers calculate the product to set count, and this model calculates the same product again only to validate it. It also assumes every component is multiplicative. Could we keep the component count as the single source of truth and remove factors and their validation?
| selection_note: str | None = None | ||
|
|
||
|
|
||
| class ScenarioDefaultRunSizeEstimate(BaseModel): |
There was a problem hiding this comment.
Could we simplify this to something like:
class ScenarioRunSizeEstimate(BaseModel):
estimated_attack_count: int | None
components: list[ScenarioRunSizeComponent]
datasets: list[ScenarioDatasetSummary]
note: str | None
| ) | ||
|
|
||
|
|
||
| def logical_seed_group_fingerprint(seed_identifiers: Sequence[SeedIdentifier]) -> str: |
There was a problem hiding this comment.
logical_seed_group_fingerprint is used by the new run plan and progress mapping, but it appears to duplicate the identity that scenario resume already uses: the atomic-attack identity plus objective_sha256. Since progress does not retrieve seed groups independently, do we need a second seed-group identifier that the service later reconstructs? Could we remove it and use the existing resume identity consistently? If it must remain, compute_seed_group_hash would match existing terminology better.
| return ScenarioRunPlan.model_validate(raw_plan) if raw_plan is not None else None | ||
|
|
||
| @staticmethod | ||
| def _result_unit_key( |
There was a problem hiding this comment.
Can we break this up or make it more readable? returning a tuple and having things named "unit_key" is hard to read
| def _result_unit_key( | ||
| *, | ||
| atomic_attack_name: str, | ||
| attack_result: Any, |
There was a problem hiding this comment.
Can we type this as an AttackResult?
| atomic_group_id = group.id | ||
| planned_group = group | ||
| break | ||
| if not seed_group_id and plan is not None and planned_group is not None: |
There was a problem hiding this comment.
do we need logic for the seed group stuff here? Isn't that in scenario (and shouldn't it live there?)
| seed_group_id = config_hash({"objective": objective}) | ||
| return atomic_group_id, seed_group_id | ||
|
|
||
| def _calculate_progress_counts( |
There was a problem hiding this comment.
should we move logic like this outside of the service?
| ] | ||
| completed = len(completed_results) | ||
| succeeded = sum(result.outcome == AttackOutcome.SUCCESS for result in completed_results) | ||
| rate = int((succeeded / completed) * 100) if completed else 0 |
There was a problem hiding this comment.
Error outcomes are excluded from the denominator. One successful unit plus one errored unit returns (total=2, completed=1, rate=100). A failed half-run therefore appears 100% successful. Keep the latest error as a non-success when calculating the rate.
| if not seed_group_id and typed_identifier is not None and typed_identifier.seed_identifiers: | ||
| seed_group_id = typed_identifier.logical_seed_group_id | ||
| atomic_group_id = atomic_attack_name | ||
| planned_group: ScenarioRunPlanAtomicGroup | None = None |
There was a problem hiding this comment.
this is also quadratic so I'm worried about it being slow
| """ | ||
| scenario_class = self._registry.get_class(scenario_name) | ||
| objective_target = ( | ||
| ScenarioRunService.resolve_target_name(target_name=request.target_name) if request.target_name else None |
There was a problem hiding this comment.
Rich agrees with this comment but it is copilot generated:
ScenarioService calls static configuration helpers on ScenarioRunService to estimate a run. Configuration resolution is shared by launch and estimation; it is not run-lifecycle state. Could we extract it into a dedicated scenario configuration resolver that both services use?
| results = self._memory.get_scenario_results(limit=limit) | ||
| items = [self._build_response_from_db(scenario_result=sr) for sr in results] | ||
| items = [ | ||
| self._build_response_from_db( |
There was a problem hiding this comment.
Rich agrees with this comment but it is copilot generated:
list_runs() hydrates all linked attack results for every returned run and then reconstructs each summary, including additional result lookups. The async route calls this synchronous work directly, and its limit has no upper bound. This can block the event loop and become expensive as run history grows. Could we use a bounded lightweight summary query and execute the database work through the thread pool?
Summary
Adds persistent versioned Scenario run plans, stable logical work identities, backend-authoritative sizing, incremental progress cursors, and corrected timing/resume semantics. Planned units deduplicate repeated logical objectives while execution attempts remain observable.
What this layer adds
ScenarioRunPlanand progress models persisted in Scenario result metadata.Visual contract
Stack
This is 1 of 7 in native GitHub Stack #2390 and targets
main.See the full implementation, screenshots, walkthrough, validation, and operational notes.
Validation
Compatibility note
Prepended-conversation request converters now default to user-role messages, and strict non-chat targets can reject excluded roles. This intentional behavior change is isolated in this foundation layer.