[Nimit Jain (Nimit3418)]

Description

Implements #3145 as a backend foundation for browsing a saved scenario's attacks in the GUI. The frontend hierarchy browser remains a separate follow-up.

• Page native roots and expand ordered children using the existing child_attack_result_ids edge store. Bound detail hydration and output, preserve parent-relative child_index separately from producer attempt_index, and report missing, cross-run, and cyclic references.
• Keep exact logical completion, raw diagnostics, producer counts, and outcome statistics independent of the displayed page. Reuse shared SDK accounting rather than loading the full progress snapshot for hierarchy reads.
• Reuse the canonical ConversationSummary and generic saved-conversation API instead of maintaining hierarchy-only transcript models. Trees defer conversation reads, selected nodes expose bounded main/pruned metadata, and transcript bodies use message-piece pagination. Adversarial and preparation contexts stay internal.
• Preserve existing unpaged conversation behavior and persisted relationships. No frontend hierarchy UI, scheduler changes, attack-policy changes, or new graph schema.

Closes #3145.

Tests and Documentation

Added SQLite/HTTP parity and pagination coverage, hierarchy model tests, and SQL Server query-compilation tests. Cases cover clipped and unclipped accounting, legacy identities, linked child errors, native root paging, child continuation, malformed references, cycles, and consistent main/pruned conversation membership. Updated the backend API documentation and GUI guide.

Fixed an order-dependent CI test by giving existing remote-media reference tests their own memory fixture. The media access policy is unchanged; the tests also pass with an ambient blob-storage configuration that reproduced the failure.

Local validation:

• uv run --no-sync pytest -q -n 4 --tb=short tests\unit\backend\test_scenario_hierarchy_pagination.py tests\unit\backend\test_saved_conversation_read_service.py tests\unit\backend\test_media_persistence.py: 107 passed, 2 skipped.
• uv run --no-sync ty check pyrit tests\unit: passed.
• Repository pre-commit hooks: passed for the feature and conflict resolutions.

The full unit-test matrix, coverage, frontend, Docker, documentation, and CodeQL workflows are running on the current PR revision. Merge remains gated on the required checks.

JupyText and provider-backed integration/attack runs were not run locally.

[Nimit Jain (Nimit3418)]

Hi Roman Lutz (@romanlutz),

Thank you for the thorough, production-grade review and the concrete edge-case reproductions. I have addressed all 9 requested changes in the latest push:

1. Canonical DB Columns (AttackResultEntry):

   • Added executed_turns and outcome_reason directly to ScenarioAttackResultDelta and select them from the first-class AttackResultEntry SQL columns in both delta statements.
   • Updated node and parent builders to project canonical values, falling back to metadata only for legacy payloads. Added real SQLite round-trip test test_sqlite_canonical_columns_round_trip.

2. SQL Server Bind Parameter Chunking:

   • Deduplicated child IDs before querying and chunked IN lookups using _MAX_BIND_VARS - 10 (max 2,000 IDs per statement) in _execute_get_scenario_attack_result_deltas_by_ids.
   • Added compilation test test_scenario_attack_result_deltas_by_ids_chunks_over_2100_ids_mssql verifying 2,500 IDs chunk and compile cleanly against MSSQL dialect.

3. Database-Level SQL Message LIMIT:

   • Implemented _fetch_bounded_conversation_messages with a distinct(sequence) query applying LIMIT directly in the database, fetching only the required message pieces.
   • Separate exact counts via COUNT queries and enforced a 50-conversation budget on related conversation loops.

4. Disconnected Cyclic Components:

   • Updated build_scenario_hierarchy to track visited results and traverse unreached components chronologically with cycle detection, isolating circular edges into unresolved_child_attack_result_ids.
   • Added mixed test test_hierarchy_disconnected_cyclic_component_mixed.

5. Bounded Hierarchy Node/Edge Budget:

   • Added max_nodes: int = 1000 parameter to build_scenario_hierarchy, get_run_hierarchy_async, and the FastAPI route (/hierarchy?max_nodes=1000). Truncation is explicitly indicated via projection.truncated = True while run-wide totals remain exact.
   • Added test test_hierarchy_bounded_node_budget.

6. Preserved Group Outcomes:

   • Carried outcomes=ag.outcomes through to planned hierarchy groups, preserving both decided-only and all-outcome success rates. Added parity test test_hierarchy_group_progress_outcomes_parity.

7. Shared Counting Policy in Unplanned Groups:

   • Reused shared count_execution_units(...) for unmatched groups, properly applying logical latest-attempt selection and populating typed ScenarioProducerCounts without custom arithmetic.
   • Added test test_hierarchy_unplanned_groups_shared_counting.

8. Bounded Diagnostic Retries:

   • Added total_retries: int and retries_truncated: bool to AttackRetrySummary, bounding individual retry lists to 100 items. Added boundary test test_hierarchy_diagnostics_cap_retries_per_item.

9. Generic Multimodal Message Evidence:

   • Created ScenarioConversationMessagePieceProjection to preserve multimodal pieces (image_path, data types) and raw stored roles (e.g. simulated_assistant) without lossy flattening. Added real SQLite test test_sqlite_bounded_multimodal_conversation_projection.

All linters passed (ruff check and ruff format), and the entire unit test suite (26,173 tests) is passing 100% green. Ready for your re-review!

[Nimit Jain (Nimit3418)]

Hi Roman Lutz (@romanlutz),

Thank you for catching those nuances! All 4 comments from the second review round have been addressed in the latest commit:

1. Select Import in memory_interface.py:

   • Imported Select from sqlalchemy. Verified clean static resolution with ruff check --select F821 and runtime annotation resolution (typing.get_type_hints) across Python 3.11+.

2. $O(N^2)$ Loop Elimination & Immediate Budget Halt:

   • Replaced the quadratic while unreached: re-scanning loop in build_scenario_hierarchy with a single linear ($O(N)$) pass, and immediately halt traversal across both roots and unreached components once total_nodes_count >= max_nodes.
   • Added benchmark test test_hierarchy_traversal_halts_immediately_on_budget_exhaustion (1,000 roots with max_nodes=1 completes in <0.01s).

3. Explicit Conversation-List Clipping:

   • Added conversations_count: int and conversations_truncated: bool to ScenarioHierarchyNode.
   • _execute_get_attack_result_conversation_projections now tracks the total conversation population, truncates to max_conversations, and sets conversations_truncated = True.
   • Added SQLite test test_sqlite_conversation_list_clipping_explicit testing 55 persisted conversations (conversations_count=55, len(conversations)=50, conversations_truncated=True).

4. Reconciled Diagnostic Retry Totals:

   • Set diagnostics.total_retries = snapshot.summary.overall.retries in build_scenario_hierarchy so that re-attempted logical units are consistently reflected across overall and diagnostic metrics.
   • Added test test_hierarchy_repeated_attempt_diagnostic_retries_consistency asserting equality.

All static type checks (ty check) and linters passed, and the entire test suite (26,179 unit tests) is green.

[Roman Lutz (romanlutz)]

Note the follow-up responses on a few open threads from above!

[Nimit Jain (Nimit3418)]

Hi Roman Lutz (@romanlutz),

Thank you for the thorough follow-ups and the heads-up on the open threads! All remaining edge cases have been resolved in the latest push:

1. executed_turns Zero-Turn vs. Metadata Fallback:

   • Implemented _resolve_executed_turns: explicitly checks if delta.executed_turns is not None to protect legitimate 0-turn results from Python falsy coercion. Falls back to metadata only when canonical data is None, with safe (ValueError, TypeError) parsing.
   • Added test test_executed_turns_zero_vs_invalid_metadata.

2. Eliminated Tuple Duck-Typing:

   • Explicitly check isinstance(conversations, ScenarioAttackResultConversations). Standard sequences (including raw tuples (c1, c2, c3)) are preserved as ordinary conversation lists.
   • Added test test_eliminate_tuple_duck_typing_raw_tuples covering 2, 3, and 4-element raw tuples.

3. Bounded Message Pieces:

   • Added max_pieces_per_message: int = 100 and pieces_truncated: bool to ScenarioConversationMessageProjection.
   • Added SQLite test test_sqlite_message_pieces_bounding verifying 150 pieces in a single message sequence are bounded to 100 with pieces_truncated=True.

4. Honest Preview Metadata on Full Hierarchy:

   • In build_scenario_hierarchy, nodes with unhydrated related conversations now report conversations_count=None and conversations_truncated=True to explicitly signal incomplete previews.
   • Added test test_unloaded_conversations_in_full_hierarchy.

5. Diagnostic Retries Scope:

   • diagnostics.total_retries now computes retry pressure across all attempted units and displayed diagnostic items, ensuring unmatched errors with retries reconcile with the diagnostic totals.
   • Added test test_diagnostic_total_retries_scope_unmatched_attempt.

6. Bounded Child References & Immediate Resolution:

   • Added Pydantic model validator bounding child_attack_result_ids to 500 with child_references_truncated=True.
   • Added max_immediate_children: int = 200 to get_run_hierarchy_node_async with children_truncated=True.
   • Added test test_immediate_children_bounding_in_run_service.

All linters (ruff check, ruff format), static typing (ty check), and the full unit test suite (26,188 passed) are 100% green. Ready for your re-review!

[Roman Lutz (romanlutz)]

A few of the earlier threads remain open (I responded on each)

[Nimit Jain (Nimit3418)]

A few of the earlier threads remain open (I responded on each)

Yes sir, I am doing the changes

[Nimit Jain (Nimit3418)]

Addressed in commit 0e7abda:

1. SQL-Level Piece Budgeting: MemoryInterface._fetch_bounded_conversation_messages now applies the piece budget directly at the database layer using func.row_number().over(partition_by=PromptMemoryEntry.sequence, order_by=(PromptMemoryEntry.timestamp.asc(), PromptMemoryEntry.id.asc())) joined on subq.c.rn <= piece_budget + 1. For a message containing 1,001 pieces, SQLite/SQL Server now only transfers and hydrates 101 rows across the wire rather than loading all 1,001 rows into Python memory, while setting pieces_truncated=True and returning the capped 100 pieces.
2. Canonical Retry Accounting: Replaced the manual max(a.total_retries, len(a.retries)) calculation in ScenarioProgressReadModel.build_scenario_hierarchy with the canonical shared ScenarioProgressReadModel.total_retry_pressure over (r.total_retries for r in snapshot.results). In the saved count/event mismatch scenario (total_retries=0 with 3 retry events), run detail, shared producer accounting, the diagnostic item, and diagnostics.total_retries now all consistently report 0.
3. Integration & Parity Tests: Added test_bounded_conversation_messages_piece_budget_applied_at_sql_layer (validating SQL row bounding with 1,001 pieces in real SQLite) and test_diagnostics_total_retries_count_event_mismatch_parity (validating parity across run summary, overall counts, and diagnostic retries).

[Nimit Jain (Nimit3418)]

Hi Roman Lutz (@romanlutz),

All feedback from the latest review round has been resolved in commit e4d0ca06:

• Bound cold delta hydration to the initial storage page size for clipped requests.
• Capped unresolved child references to 500 in both full hierarchy and node detail projections, backed by Pydantic model validation.
• Bound conversation statistics queries to visible preview nodes and eliminated false zeroes on unqueried/failed stats.
• Added comprehensive edge-case parity tests matching all review scenarios.

[Nimit Jain (Nimit3418)]

Hi Roman Lutz (@romanlutz),

I have addressed all feedback from your latest review with a focus on strict zero-inference, exact cold/warm cache parity, and eliminating arbitrary query limits. Here is a summary of the architectural updates in commit 04d713ea:

1. Strict Zero-Inference on Disconnected Roots:

   • For unreached fallback nodes where no parent edge is resolved, child_index is strictly set to None instead of inferring attempt_index - 1. This restores complete consistency between get_run_hierarchy_async and get_run_hierarchy_node_async.

2. Targeted Visible-Parent Resolution (No Blind LIMIT 500):

   • Replaced the global parent query with get_scenario_parent_deltas_for_child_ids_async, resolving parents dynamically only for visible root candidates along the candidate chain.
   • Refined the metadata pattern to %child_attack_result_ids%["% so target-facing records with empty child arrays ([]) no longer trigger false matches.

3. Cold Path (max_nodes=1) vs. Warm Cache 100% Parity:

   • Outcomes Breakdown: Extended the SQL history aggregate statement to compute outcome breakdown counters (outcome_successes, outcome_failures, outcome_errors, outcome_undetermined), passed to compute_outcome_statistics so cold requests provide the exact same outcome breakdown as warm cache.
   • Per-Group Aggregates: Grouped SQL history aggregates by unit group ID, mapping both planned and unplanned groups (via config_hash) to provide exact per-group counts (completed, succeeded, errors, retries, and outcomes).
   • Raw Diagnostics: Diagnostic totals (failed_attacks_count, attack_retries_count, and total_retries) now reconcile against database aggregate totals (raw_total_errors, raw_retrying_attacks_count, and raw_total_retries).

4. Bounded Keyset Recount Without Hydrating Full ScenarioResults:

   • Refactored _recount_with_sdk_statistics_async to stream lightweight deltas using keyset pagination (get_scenario_attack_result_deltas_async with limit 500) and count_execution_units, promoting typed AtomicAttackIdentifier for seed grouping without loading full ORM ScenarioResult entities or prompt histories.

5. Exhaustive Parity & Boundary Testing:

   • Added 5 new boundary test cases in tests/unit/backend/test_scenario_hierarchy_parity.py covering:
     • 500 empty-array children preceding a parent.
     • 501 orchestration parents with an outer root parent.
     • Unreached node zero-inference parity (child_index=None).
     • 501 planned units with tail error + unmatched tail error cold vs. warm parity.
     • 501 unplanned group attempts cold vs. warm parity.
     • 501-result legacy recount delta keyset paging without full result hydration.
   • All 48 parity tests, 1,413 memory tests, and 2,331 backend tests pass. Type checking (ty check) and linting (ruff check) report zero errors.

[Roman Lutz (romanlutz)]

The two remaining open review threads are:

• Counting and retry totals
• Root discovery with bounded parent reads

Both have concrete examples of the remaining issues in the linked follow-ups.

[Roman Lutz (romanlutz)]

Summary

Reviewed a1b11089. The earlier examples are fixed, but the bounded-read and parent-context guarantees still have blocking gaps.

Findings

🔴 Full-run hydration is back

Location: pyrit/backend/services/scenario_run_service.py:2546-2549

Removing the recount budget restores exact legacy counts by loading every attempt. A cold one-node request over 2,001 saved legacy attempts hydrates 2,501 deltas across six result-page reads. Please preserve exact shared statistics without requiring full-run attempt-detail hydration. Details in the existing thread.

🔴 Root guards fail at their limits

Location: pyrit/backend/services/scenario_progress_read_model.py:909-914; pyrit/backend/services/scenario_run_service.py:2525-2538

An 11-parent chain emits parent 10 as a root even though it is explicitly marked unverified. At the 2,500-parent budget, newly discovered parents also remain eligible as roots without checking their incoming links. Please preserve unknown parent context through the fallback and budget-exhaustion paths rather than presenting these nodes as confirmed roots. Reproductions in the existing thread.

🔴 SQL Server's parameter limit is exceeded

Location: pyrit/memory/memory_interface.py:7062-7067

All exclusion chunks share one statement, but only 200 exclusion parameters are reserved. The actual Azure SQL query compiles to 2,201 parameters for 1,100 exclusions and 1,100 candidates, above SQL Server's 2,100 limit. Later parent paging with the default node limit can produce 2,501 parameters. Please budget all statement parameters, including the complete exclusion list. The inline comment includes the details.

Verdict

REQUEST CHANGES. Both existing threads remain open; the SQL Server finding is included as an inline comment in this review.

[Nimit Jain (Nimit3418)]

I have pushed commit dfb3ce37 addressing all three points with comprehensive real-world scale tests and verified the entire test suite passes locally (3,762 passed, 0 failures, ruff formatting and linting clean):

1. Lightweight Recount Projection: Introduced ScenarioAttemptSeedSummary and get_scenario_recount_summaries_async so recount no longer hydrates full deltas, scores, or retry events, keeping recount bounded without sacrificing count accuracy.
2. Root Boundary Guards: Removed the fallback that bypassed unverified guards, pruned all unverified descendants from the disconnected-component loop, and fixed budget check ordering in parent discovery.
3. SQL Server Bind Limits: Dynamically budgeted statement parameters to strictly guarantee len(params) <= _MAX_BIND_VARS (2,000) under any exclusion size, verified via MSSQL compilation tests.

[Roman Lutz (romanlutz)]

Summary

Reviewed dfb3ce3. The full-detail hydration and SQL Server parameter-limit findings are addressed and their threads are resolved. Two correctness issues remain.

Findings

🔴 Parent discovery still invents roots

Location: pyrit/memory/memory_interface.py:7175-7178,7232-7243

The Python-only exclusion fallback filters known parents after the SQL row limit. A short filtered page is then treated as complete. In a 1,002-result example, this hides a saved parent and makes the cold hierarchy emit its leaf as a root, while the warm hierarchy emits an actual parent root. Please filter before limiting or explicitly track incomplete parent coverage. Reproduction in the existing thread.

🔴 Retry-event-only attempts disappear from diagnostic totals

Location: pyrit/backend/services/scenario_run_service.py:1771-1774

The lightweight recount ignores saved retry events when total_retries=0. A cold hierarchy reports 0 retrying attacks, while SQL and the warm hierarchy report 1. Please preserve the SQL count or carry a retry-presence flag. The inline comment includes the reproduction and adjacent controls.

Verdict

REQUEST CHANGES. The parent-discovery thread remains open, and the diagnostic finding is included as an inline comment in this review.

[Nimit Jain (Nimit3418)]

I have pushed commit 1d14bd23 resolving both remaining findings with comprehensive regression tests. All 2,352 backend tests, 68 hierarchy parity tests, and 56 Azure SQL memory tests pass locally with clean linting and formatting.

1. Parent Discovery Keyset Pagination & True Completion

• The Issue: When SQL exclusions were omitted or partial due to bind-parameter limits, a single-query chunk scan returned known rows before new parents. Filtering in Python yielded a short page (< 500), which scenario_run_service.py erroneously treated as DB exhaustion, stranding leaf 0's parent in the 1,002-result scenario.
• The Fix:
  • In _execute_get_scenario_parent_deltas_for_child_ids, chunk queries now loop using an ascending keyset cursor (AttackResultKeysetCursor(timestamp, id)). If a query returns a full page (len(rows) > scan_page_size) but Python filtering leaves len(unique_deltas) < limit, it continues seeking past known rows until either limit is satisfied or the database genuinely returns < scan_page_size.
  • Updated get_scenario_parent_deltas_for_child_ids_async to return tuple[list[ScenarioAttackResultDelta], bool] ((new_parents, has_more)), standardizing it with PyRIT's other bounded projections.
  • In scenario_run_service.py, replaced the len(new_parents) < page_limit break condition with explicit database completion (if not has_more: break). If has_more is true but no new parents are found within budget, layer_clipped is set and unverified candidates are guarded.
• Verification: Added test_1002_results_500_leaves_501_parents_leaf_0_not_emitted_as_root_parity matching the exact 1,002-result SQLite case (500 leaves, 501 parents referencing leaf 1 covering leaves 1..499, 1 parent referencing leaf 0). The cold max_nodes=1 hierarchy no longer emits leaf 0 as a false root and matches the warm hierarchy.

2. Retry-Event Diagnostics Preservation

• The Issue: _recount_with_sdk_statistics_async evaluated sum(1 for d in all_summaries if d.total_retries > 0). When an attempt had total_retries == 0 but contained persisted RetryEvent entries in retry_events_json, recount zeroed out the diagnostic count.
• The Fix:
  • Added has_retries: bool = False to ScenarioAttemptSeedSummary.
  • Selected has_retries in _execute_get_scenario_recount_summaries via SQL:
    CASE WHEN (COALESCE(total_retries, 0) > 0 OR retry_events_json IS NOT NULL) THEN 1 ELSE 0 END.
    This evaluates retry presence directly in the database without inflating memory or hydrating JSON payloads.
  • In _recount_with_sdk_statistics_async, counted sum(1 for d in all_summaries if d.has_retries or d.total_retries > 0), keeping the diagnostic total aligned with the SQL aggregate and warm hierarchy.
• Verification: Added test_500_results_followed_by_retrying_attempt_diagnostic_count_parity parameterized across total_retries=0, 1, and 3. Confirmed that 500 ordinary results followed by 1 result with total_retries=0 and 1 saved RetryEvent reports attack_retries_count=1 across cold hierarchy, warm hierarchy, and SQL aggregate.

[Roman Lutz (romanlutz)]

One issue is still open: parent discovery can report a child as a root. With 500 leaves and 501 parents, the final query chunk stops at the result limit but returns has_more=False while one parent is still unprocessed. The cold hierarchy then shows leaf 499 as a root, while the warm hierarchy shows an actual parent root. The linked thread includes the reproduction and suggested fix.

Could you also merge the latest main into this branch and resolve the conflicts? GitHub currently reports merge conflicts with main.

[Nimit Jain (Nimit3418)]

PR Update & Upstream Sync

1. Parent-Discovery Boundary Condition Resolved:

   • Accounted for unconsumed rows on the current page (has_unprocessed_in_page) when a chunk query is truncated by the caller's limit.
   • Verified via parameterized regression test test_chunk_boundary_unprocessed_rows_500_leaves_parents_leaf_499_parity.

2. Full upstream/main Sync & Architectural Alignment:

   • Merged the latest upstream/main cleanly.
   • Unified completed_units and outcome aggregations with the sequential step execution model introduced in FEAT: Run one technique against an objective with technique.execute #3118.
   • Maintained keyset cursor delta polling for edited attempts (Scenario progress double-counts an attack result after its timestamp is bumped #3183).

3. Verification:

   • tests/unit/backend/test_scenario_hierarchy_parity.py: 72/72 passed.
   • tests/unit/memory/test_azure_sql_memory.py: 88 passed (1 live cloud integration skipped).
   • Full tests/unit/backend/ suite: 2,890 passed, 0 failed.
   • ruff check and ruff format --check: 0 errors.

[Roman Lutz (romanlutz)]

Reviewed the simplified backend on this revision. Bounded hierarchy reads no longer depend on full progress hydration, exact accounting stays shared, and saved conversation reads consistently expose main/pruned only. The remaining accounting findings are addressed.

[Nimit Jain (Nimit3418)]

Thank you so much for the detailed review, guidance, and help with the final integration merges Roman Lutz (@romanlutz)! Really excited to see this land in PyRIT.