Repository navigation
FEAT: Shared scan result building blocks and ScenarioRunPage split (part 2) - #4031
Richard Lundeen (richlundeen) wants to merge 13 commits into
Conversation
- GET /api/attacks/{id}/findings lists findings linked to an attack across Operations,
backed by a new FindingEvidenceEntries.attack_result_id index (Alembic b5d7f9a1c3e6).
- Attack read APIs (summary, metadata, conversations, messages) resolve scan attacks that
are still running through a read-only LiveAttackLookup provided by ScenarioRunService.
Saved results always win; summaries/metadata expose in_progress and scenario_result_id.
- ScenarioActiveConversation.conversation_ids lists every objective conversation so far.
- ScenarioProgressScore carries score_category for progress results and hierarchy nodes.
- Frontend types and attacksApi.getAttackFindings for later layers.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Split ScenarioRunPage into section components and hooks, and add the shared blocks the scan results redesign builds on: OperationParentLink, ScanStatusBadge, OutcomeBadge progress labels and score popover, ScoreDetails over a narrow ScoreSummary, DetailFacts, IdentityDisclosure, AttackActivity, formatAttemptTiming, useAttackFindings, and AttachToFindingsButton. Existing pages adopt them only where output is unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The popover-enabled badge’s accessible name omits its visible outcome, making success, failure, and error indistinguishable to screen-reader users without opening each popover.
1 open issue
What changed in this PR
This adds the frontend foundations for the scan-results redesign, building on #4030’s backend APIs. It splits ScenarioRunPage into focused sections and hooks, following #3526’s approach, while preserving the existing layout and improving attack-dialog focus return.
Changes:
- Extracts run controls, attack selection, navigation, and page sections.
- Centralizes status badges, score details, identity disclosure, and reusable result components.
- Adds findings-loading and attachment helpers for the next redesign layer.
| File | Description |
|---|---|
| tests/unit/memory/test_score_participation_migration.py | Allows a newer single migration head. |
| tests/unit/memory/test_findings.py | Tests cross-operation attack findings. |
| tests/unit/memory/test_finding_evidence_attack_index_migration.py | Tests index migration and backend compatibility. |
| tests/unit/memory/memory_interface/test_interface_scenario_progress.py | Verifies progress score categories. |
| tests/unit/backend/test_scenario_resume.py | Tests live-to-saved attack reads. |
| tests/unit/backend/test_live_attack_reads.py | Covers live reads and attack findings APIs. |
| pyrit/models/scenario_progress.py | Adds categories and objective conversation IDs. |
| pyrit/memory/memory_models.py | Declares the attack-evidence lookup index. |
| pyrit/memory/memory_interface.py | Shares reference paging and exposes findings/categories. |
| pyrit/memory/alembic/versions/b5d7f9a1c3e6_index_finding_evidence_attack_result.py | Adds the attack-evidence lookup index. |
| pyrit/backend/services/scenario_run_service.py | Exposes active attack lookup and conversations. |
| pyrit/backend/services/saved_conversation_read_service.py | Accepts an injectable reference reader. |
| pyrit/backend/services/operation_service.py | Lists findings linked to an attack. |
| pyrit/backend/services/live_attack_reader.py | Defines read-only live attack projections. |
| pyrit/backend/services/attack_service.py | Supports live attacks in read endpoints. |
| pyrit/backend/routes/attacks.py | Adds the attack findings endpoint. |
| pyrit/backend/README.md | Documents live reads and findings contracts. |
| pyrit/backend/models/operations.py | Defines attack findings responses. |
| pyrit/backend/models/attacks.py | Adds live-state and scan provenance fields. |
| pyrit/backend/mappers/attack_mappers.py | Maps live-state and provenance fields. |
| frontend/src/utils/scoreSummary.ts | Normalizes score data for shared presentation. |
| frontend/src/utils/scoreSummary.test.ts | Tests score adapters and fallback values. |
| frontend/src/types/index.ts | Adds shared display and API contracts. |
| frontend/src/services/api.ts | Adds attack findings retrieval. |
| frontend/src/services/api.test.ts | Tests findings requests and failures. |
| frontend/src/hooks/useScenarioRunControls.ts | Extracts rerun, cancel, and resume controls. |
| frontend/src/hooks/useScenarioRunAttackDetails.ts | Extracts attack selection and focus restoration. |
| frontend/src/hooks/useAttackFindings.ts | Loads and refreshes linked findings. |
| frontend/src/hooks/useAttackFindings.test.tsx | Tests findings loading and state transitions. |
| frontend/src/components/ScenarioSuites/ScenarioSuiteExecutionPage.tsx | Adopts shared scan status badges. |
| frontend/src/components/Scenarios/ScenarioTechniqueSection.tsx | Uses shared identity disclosure styles. |
| frontend/src/components/Scenarios/ScenarioRunProgress.tsx | Extracts progress and timing display. |
| frontend/src/components/Scenarios/ScenarioRunPage.tsx | Composes extracted sections and hooks. |
| frontend/src/components/Scenarios/ScenarioRunOverview.tsx | Extracts scan summary facts. |
| frontend/src/components/Scenarios/ScenarioRunObjectivesSection.tsx | Extracts dataset and objective results. |
| frontend/src/components/Scenarios/ScenarioRunNotices.tsx | Extracts errors and progress notices. |
| frontend/src/components/Scenarios/scenarioRunNavigation.ts | Centralizes run back-link rules. |
| frontend/src/components/Scenarios/scenarioRunNavigation.test.ts | Tests back-link destinations and labels. |
| frontend/src/components/Scenarios/ScenarioRunHeader.tsx | Extracts title, status, and controls. |
| frontend/src/components/Scenarios/ScenarioRunDialogs.tsx | Adds the extracted cancellation dialog. |
| frontend/src/components/Scenarios/ScenarioRunConversations.tsx | Extracts executing and saved conversations. |
| frontend/src/components/Scenarios/ScenarioRunConfigurationSection.tsx | Extracts saved configuration disclosure. |
| frontend/src/components/Scenarios/AttackExecutionTable.tsx | Uses shared outcome label styles. |
| frontend/src/components/Scenarios/AttackDetailsDialog.tsx | Extracts attack details presentation. |
| frontend/src/components/Scenarios/AttackActivity.tsx | Adds a reusable activity graphic. |
| frontend/src/components/Scenarios/AttackActivity.test.tsx | Tests activity labels and loading states. |
| frontend/src/components/Scenarios/AttackActivity.styles.ts | Styles activity with reduced-motion support. |
| frontend/src/components/Scenarios/AtomicAttackGroupsSection.tsx | Extracts expandable attack groups. |
| frontend/src/components/ScanStatusBadge.tsx | Centralizes lifecycle labels, colors, and icons. |
| frontend/src/components/ScanStatusBadge.test.tsx | Tests status labels and optional icons. |
| frontend/src/components/Registry/RegistryIdentityRow.tsx | Reuses shared disclosure components. |
| frontend/src/components/Registry/RegistryIdentityRow.styles.ts | Retains row-specific styling. |
| frontend/src/components/Registry/IdentityDisclosure.tsx | Adds reusable identity JSON disclosure. |
| frontend/src/components/Registry/IdentityDisclosure.test.tsx | Tests disclosure behavior and click isolation. |
| frontend/src/components/Registry/IdentityDisclosure.styles.ts | Centralizes disclosure styling. |
| frontend/src/components/OutcomeBadge.tsx | Adds pending states and score popovers. |
| frontend/src/components/OutcomeBadge.test.tsx | Covers new labels and popover behavior. |
| frontend/src/components/OutcomeBadge.styles.ts | Styles pending states and score triggers. |
| frontend/src/components/Operations/OperationParentLink.tsx | Adds reusable parent-operation navigation. |
| frontend/src/components/Operations/OperationParentLink.test.tsx | Tests links and missing-operation behavior. |
| frontend/src/components/Operations/OperationParentLink.styles.ts | Styles parent-operation links. |
| frontend/src/components/Home/Home.tsx | Adopts shared scan status badges. |
| frontend/src/components/DetailFacts.tsx | Adds reusable description-list components. |
| frontend/src/components/DetailFacts.test.tsx | Tests label/value semantics. |
| frontend/src/components/DetailFacts.styles.ts | Adds responsive fact-list layout. |
| frontend/src/components/Chat/ScoreDetails.tsx | Extracts score presentation using ScoreSummary. |
| frontend/src/components/Chat/ScoreDetails.test.tsx | Tests score details and empty sections. |
| frontend/src/components/Chat/ScoreDetails.styles.ts | Extracts shared score-detail styling. |
| frontend/src/components/Chat/MessageList.tsx | Uses shared score details and adapters. |
| frontend/src/components/Chat/MessageList.styles.ts | Removes extracted score-detail styles. |
| frontend/src/components/Chat/FindingEvidenceDialog.tsx | Reports successful attachments via callback. |
| frontend/src/components/Chat/AttachToFindingsButton.tsx | Wraps attachment dialog and focus return. |
| frontend/src/components/Chat/AttachToFindingsButton.test.tsx | Tests attachment, cancellation, and disabled states. |
| frontend/src/components/Chat/AttachToFindingsButton.styles.ts | Applies touch-target sizing. |
| frontend/src/components/AttackResults/attackAttemptFormatting.ts | Adds retry and combined timing formatting. |
| frontend/src/components/AttackResults/attackAttemptFormatting.test.ts | Tests retry and timing output. |
Review effort: Balanced
Integrate main 25417ff without rewriting the existing stack history. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Limit running-state guarantees to attack summary and metadata reads without expanding conversation/message schemas or removing score details. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Carry forward the verified running-state documentation correction and the predecessor main-integration lockfile state. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve the visible verdict in score-trigger accessible names, cover saved and pending outcomes plus custom labels, and prove populated findings survive refresh before replacement. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The accessible-name concern in this review is fixed in 343e600. The badge keeps its visible outcome in the score action name; the existing inline thread has the detailed correction. |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Integrate confirmed CI base c979a97 without rewriting either Alembic branch. The reproduced dual-head conflict is repaired in the following commit before publication. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve both published revision histories and restore singular-head backend startup. Cover fresh, common-parent, either-branch and dual-head upgrades with preserved data, idempotency, downgrade/re-upgrade and SQLite/SQL Server merge semantics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Carry forward the reviewed no-DDL Alembic merge revision and pinned upstream integration while preserving the foundations repairs and published visual walkthrough. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Richard Lundeen (richlundeen)
left a comment
Richard Lundeen (richlundeen)
left a comment
There was a problem hiding this comment.
Automated review completed. No blocking concerns remain. This review records source coverage; it does not authorize merge.
Richard Lundeen (richlundeen)
changed the title
Integrate microsoft/PyRIT main at 8253694. Preserve live attack reads, findings, score provenance and both migration histories while adopting canonical public imports. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Behnam (behnam-o)
left a comment
Behnam (behnam-o)
left a comment
There was a problem hiding this comment.
Reviewed this layer's own changes (d69f177..36ca73f); part 1 is reviewed on #4030. The ScenarioRunPage split and the shared badge, score and identity components look behavior-preserving, and I didn't find any security issues.
Requesting changes because a large part of this PR has no caller until #4032: new components and hooks, plus new options on shared components that are already in use. Since each PR merges into main on its own, please move those into #4032 next to their callers.
This was a static review; I didn't run the code or tests.
| /** Opens the finding picker to attach one attack as evidence, returning focus to the button. */ | ||
| export default function AttachToFindingsButton({ | ||
| attackResultId, defaultOperationId = null, disabled = false, onAttached, | ||
| }: AttachToFindingsButtonProps) { |
There was a problem hiding this comment.
[MEDIUM] Part-3 building blocks have no caller in this PR
Outside tests, nothing in this PR renders AttachToFindingsButton. The same goes for OperationParentLink, DetailFacts, AttackActivity, useAttackFindings, the new helpers in attackAttemptFormatting.ts (lines 79-93), and the new onAttached prop on FindingEvidenceDialog, which the live dialog in ChatWindow doesn't pass. The PR description itself says the new blocks are unused until layer 3. Since each PR merges into main on its own, please drop them here and add them in #4032 with their callers.
There was a problem hiding this comment.
Confirmed at 36ca73f: the current run page and Chat attachment flow do not use these blocks. I also checked the actual consumers in #4032 at 041171e: the redesigned attack details connect AttachToFindingsButton.onAttached to refreshFindings, and the new header uses OperationParentLink. Moving the unused pieces and options there preserves the planned workflow while leaving the consumed status/score/identity extractions, page split, and focus-return fix here. Independent verification supports this scope reduction. The fix loop stopped at its inclusion gate; it made no source changes and did not approve or merge the PR.
There was a problem hiding this comment.
Done in 8a9977b. This PR no longer adds AttachToFindingsButton, OperationParentLink, DetailFacts, AttackActivity, useAttackFindings, formatAttemptTiming / formatRetries, or FindingEvidenceDialog's onAttached. They're now added in #4032 next to their callers (attack details, the run header, the progress area, and the results table). Merging this branch into #4032 kept #4032's code unchanged apart from the technique identity change in the LOW thread.
| if (!score && !scoreFallback) return badge | ||
|
|
||
| return ( | ||
| <Popover withArrow> | ||
| <PopoverTrigger disableButtonEnhancement> | ||
| <Button appearance="transparent" className={styles.scoreTrigger} aria-label={`${displayLabel}: ${scoreButtonLabel}`}> | ||
| {badge} | ||
| </Button> | ||
| </PopoverTrigger> | ||
| <PopoverSurface> | ||
| {score ? <ScoreDetails score={score} /> : <Text>{scoreFallback}</Text>} | ||
| </PopoverSurface> | ||
| </Popover> | ||
| ) |
There was a problem hiding this comment.
[MEDIUM] Shared components gain options that only part 3 uses
None of the 7 production OutcomeBadge call sites passes score, scoreFallback or scoreButtonLabel, so this popover branch can't be reached yet. It still widens a badge used on 7 screens and couples it to Chat/ScoreDetails. The same goes for scoreSummaryFromProgressScore, the default IdentityDisclosure export, chevronPosition and IdentityJson's className, which nothing outside tests uses. Keeping labelStyle, ScoreSummary/ScoreDetails (used by MessageList) and the two extracted identity parts makes sense; please move the rest to #4032.
There was a problem hiding this comment.
Done in 8a9977b. OutcomeBadge here keeps only labelStyle. The in_progress / preparing states, AttackDisplayOutcome, and the score popover (score, scoreFallback, scoreButtonLabel, with its ScoreDetails import) moved to #4032. So did scoreSummaryFromProgressScore, the default IdentityDisclosure export, and chevronPosition. ScoreSummary/ScoreDetails (used by MessageList) and IdentityDisclosureButton / IdentityJson stay. IdentityJson's className also stays, because the technique section now passes it (see the LOW thread).
| }: ScenarioTechniqueSectionProps) { | ||
| const styles = useScenarioDetailStyles() | ||
| const identityStyles = useRegistryIdentityRowStyles() | ||
| const identityStyles = useIdentityDisclosureStyles() |
There was a problem hiding this comment.
[LOW] This section still has its own JSON reveal
The PR description says the app now has one JSON reveal implementation, but this section only switches to IdentityDisclosure's internal details/json styles and keeps its own role="region" + <pre> markup (lines 177-181). It already behaves differently from IdentityJson (it doesn't scroll into view), and restyling IdentityJson would silently change it. Could this render IdentityJson when an identifier exists (keeping the fallback text inline), or could the description be narrowed?
There was a problem hiding this comment.
Changed to render IdentityJson in 8a9977b. When a technique has an identifier, the section uses the shared region, so it scrolls into view like registry identities, and later IdentityJson changes apply here too. The fallback text ("Baseline identity is available after launch." / "Identity was not recorded…") stays inline in its own region. A new ScenarioDetail test covers a recorded technique identity.
Keep the shared pieces this PR already uses and add the rest with their callers in the redesign: AttachToFindingsButton, OperationParentLink, DetailFacts, AttackActivity, useAttackFindings, attempt timing helpers, the FindingEvidenceDialog onAttached callback, OutcomeBadge pending states and score popover, scoreSummaryFromProgressScore, and the standalone IdentityDisclosure. Scenario technique identities now render the shared IdentityJson region.
Description
Layer 2 of 3 for the scanner results redesign. It splits
ScenarioRunPageinto sections and gives the existing pages one shared implementation each for scan status, outcome labels, score details, and identity JSON. Existing pages look the same. Layer 3 (#4032) builds the redesign on these pieces.Everything here has a caller in this PR. Building blocks that only the redesign uses are added in #4032, next to their callers.
ScenarioRunHeader,ScenarioRunOverview,ScenarioRunNotices,ScenarioRunProgress,ScenarioRunConversations,AtomicAttackGroupsSection,ScenarioRunObjectivesSection,ScenarioRunConfigurationSection,AttackDetailsDialog, andCancelScanDialog. Rerun, cancel, and resume state lives inuseScenarioRunControls. Route-addressed attack selection and focus return live inuseScenarioRunAttackDetails. The back-link rules moved toscenarioRunBackLink. The baseline/attack comparison frommainstays on completed runs.STATE_COLORS(run page),SCAN_STATUS_*(Home), and*_BADGE_COLORS+statusIcon(suite execution page). The suite page keeps its "In progress" wording throughlabel.labelStyle('success'→ True/False,'outcome'→ Success/Failure).AttackExecutionTableuses it instead of hand-built labels.MessageListintoChat/ScoreDetailsand takes a narrowScoreSummary, built from a message score byscoreSummaryFromBackendScore.IdentityDisclosureButtonandIdentityJson.RegistryIdentityRowand the scenario launch technique identities use them, so the app has one JSON reveal implementation. Technique identities now scroll into view when revealed, like registry identities.Behavior note: closing attack details now always returns focus to the control that opened them. Before, opening an attack from an "Executing" row could send focus back to an earlier trigger.