feat: Add bounded Scenario result hierarchy projections (#3145)

[Nimit Jain (Nimit3418)]

Description

This PR implements bounded Scenario result hierarchy projections as requested in #3145. It provides a typed, bounded read-only projection resolving parent/child orchestration relationships, ordered direct child references, and generic conversation projections without requiring full-run result or message hydration.

Key Changes:

• Typed Hierarchy Models (pyrit/models/scenario_hierarchy.py):
  • Added ScenarioHierarchyProjection, ScenarioHierarchyNode, ScenarioOrchestrationParentSummary, ScenarioHierarchyGroup, ScenarioHierarchyDiagnostics, and generic read-only ScenarioConversationProjection.
  • Preserves parent-relative child_index (0-indexed position), producer attempt_index (1-indexed), and explicit fallback to AttackResultRole.UNKNOWN.
• Bounded Batched Memory Reads (pyrit/memory/memory_interface.py):
  • Added get_scenario_attack_result_deltas_by_ids_async executing a single batched query matching id IN (...) and attribution_parent_id == scenario_uuid to ensure cross-run isolation.
  • Added get_attack_result_conversation_projections_async to fetch conversation stats and bounded messages without hydrating unrelated run data.
• Tree Assembly & Safety Guards (pyrit/backend/services/scenario_progress_read_model.py):
  • Single-node detail queries in get_run_hierarchy_node_async fetch only the targeted node and immediate children.
  • Cycle detection via active_path and recursion depth guard (MAX_HIERARCHY_DEPTH = 10).
  • Missing or cross-run references are recorded in unresolved_child_attack_result_ids without failing the run.
  • Diagnostic items (failed_attacks, attack_retries) are capped at 100 while preserving exact total counters.
  • Technique identity is safely projected once per atomic group via ag.technique_details.
  • Reuses shared producer_counts and OutcomeStatistics from FEAT: Add role-aware Scenario target-attempt accounting (#3043) #3062 and FEAT: Attack analytics SDK #3059 unchanged.
• REST Endpoints (pyrit/backend/routes/scenarios.py):
  • Added GET /api/scenarios/runs/{scenario_result_id}/hierarchy.
  • Added GET /api/scenarios/runs/{scenario_result_id}/hierarchy/nodes/{attack_result_id}.

Closes #3145

Testing

• Added unit tests in tests/unit/models/test_scenario_hierarchy_models.py.
• Added MSSQL compilation tests in tests/unit/memory/test_azure_sql_memory.py.
• Added SQLite parity tests in tests/unit/backend/test_scenario_hierarchy_parity.py covering ordinary results, nested sequential attacks, cyclic reference guards, retained FIX: preserve child-result links when execution fails in SequentialAttack #3048 failure links, missing child references, diagnostic boundary truncation, bounded single-node reads, and REST routes.
• Ran make unit-test: all 26,162 tests passed.
• Ran ruff check and ruff format: all checks passed cleanly.

[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.

[Roman Lutz (romanlutz)]

🔴 Must fix: Do not identify roots from an incomplete delta page.

The new max_deltas limit means snapshot.results can omit a parent saved after its children. This code then treats those children as roots because it has not loaded the parent's stored references.

I persisted 500 children followed by their Sequential-style parent, with all 500 ordered child IDs on that parent. A cold max_nodes=1 request returns the first child in group.roots, with parent_attack_result_id=None and child_index=None. After reading progress to warm the cache, the identical hierarchy request returns the actual parent instead. No saved data changes between those requests.

Please obtain the necessary run-scoped root/parent references through a bounded read before choosing roots, or explicitly represent unavailable parent context instead of presenting these children as independent roots. A real persisted case with the parent beyond the delta budget would catch this.

[Nimit Jain (Nimit3418)]

Resolved in commit 7c489c5.

Added get_scenario_orchestration_parent_deltas_async to MemoryInterface to query run-scoped orchestration parent deltas when progress_snapshot.has_more is True. In build_scenario_hierarchy, these parent deltas are merged before identifying roots, so that parent references saved after their children (such as in sequential compound attacks) are known regardless of cold delta pagination limits.

Furthermore, root selection now strictly excludes results that carry an attempt_index (which designates a child attempt dispatched by an orchestration parent), and the unreached fallback pass preserves the child's attempt-relative position (fallback_child_index = attempt_index - 1) instead of presenting it as an independent root with child_index = None. Added test_cold_hierarchy_projection_preserves_parent_beyond_delta_page_limit matching your exact reproduction (500 children followed by parent) to guarantee parity between cold and warm requests.

[Roman Lutz (romanlutz)]

Rechecked 7c489c5. The original example with 500 plain children followed by their parent is fixed.

The new parent read is still only a prefix, though. _execute_get_scenario_orchestration_parent_deltas uses LIKE '%child_attack_result_ids%' and limits the result to _MAX_BIND_VARS, which is 500 on SQLite. Giving those same 500 target-facing children the valid metadata {"child_attack_result_ids": []} fills all 500 parent-query slots with the children. Their actual later parent is not returned. The cold one-node hierarchy again emits child 0 as a root with no parent; warming progress makes it return the actual parent.

I also reproduced the missing outer root with 501 real orchestration parents, so filtering empty arrays alone will not fix the completeness problem.

Please resolve the parent context needed for the visible nodes through a bounded run-scoped read, without treating this limited parent query as a complete reference set. Increasing the row cap would only move the boundary. Leaving this thread open.

_{Auto-replied by the GitHub Copilot app}

[Roman Lutz (romanlutz)]

Rechecked 04d713e. The empty-child-array case and the many-parent examples from the previous reply now find the actual outer root.

There is still a gap for cyclic components, which #3145 explicitly includes. I persisted 500 results forming a cycle, followed by an outer parent whose stored child list points to the first cycle member. Every prefix result is already referenced, so current_candidates is empty and the targeted parent query never runs. A cold max_nodes=1 request presents the first cycle member as a root with no parent; warming progress makes the identical request return the actual outer parent.

Could we also check incoming parent context for unreached components before treating them as roots, within the read budget? The fallback should not imply that an omitted parent does not exist. Leaving this thread open for that remaining case.

_{Auto-replied by the GitHub Copilot app}

[Nimit Jain (Nimit3418)]

Great catch on cyclic components where every node within the cycle is referenced as a child by another node.

We've enhanced the parent candidate resolution in get_scenario_hierarchy_async:

• A reachability traversal now runs from unreferenced_roots.
• If unreferenced_roots is empty (a completely closed cyclic loop in the initial page) or if isolated cyclic components exist that aren't reachable from the unreferenced roots, candidate discovery initializes with or expands into unreached_cyclic nodes up to the parent budget.
• Added comprehensive unit test test_500_cyclic_results_with_outer_parent_at_501_parity validating that a 500-result cyclic loop with an external parent at index 501 is discovered identically on both warm and cold paths.

[Nimit Jain (Nimit3418)]

Good catch on both boundary conditions. I have resolved both:

1. Depth boundary fallback: I removed the if not root_results: fallback in build_scenario_hierarchy that was stripping unverified_set. In the disconnected-component pass, I compute unverified_descendants (all unverified candidates plus their downstream reachable subtrees) and exclude them from being emitted as roots (parent_id=None). Unverified nodes and their children are never promoted to apparent roots, and hierarchy_truncated is unconditionally set to True.
2. 2,500-parent budget boundary: I reordered the parent discovery loop in scenario_run_service.py so len(parent_deltas) >= parent_budget is evaluated before checking len(new_parents) < page_limit. When clipped by budget, both layer_candidates and next_layer_candidates are added to unverified_candidate_ids. I also added an invariant pass marking any node in all_results_map unverified if it has no parent in the loaded graph and its parents were not completely checked in the DB.
3. Added regression tests: test_11_parent_chain_boundary_guards_depth_cutoff_mssql_and_sqlite (verifying no intermediate parent in an 11-parent chain is emitted as a root with max_nodes=1) and test_parent_budget_boundary_guards_unverified_parents (verifying none of the 2,500 boundary parents are emitted as roots).

[Roman Lutz (romanlutz)]

Rechecked dfb3ce3. The depth fallback is fixed: the 10-, 11-, and 12-parent cases no longer promote unverified nodes. There is still a false-root case caused by the new Python-only exclusion fallback in memory_interface.py:7175-7178,7232-7243.

The SQL row limit is applied before Python drops already-known results. That can make a clipped parent search look complete. Here is a 1,002-result SQLite example:

• Save 500 leaves.
• Save 501 parents that all reference leaf 1 and, between them, cover leaves 1 through 499.
• Save one final parent referencing leaf 0.

The first parent page returns 500 parents. On the next read, SQL exclusions are disabled, so its 501-row limit returns the same 500 known parents plus one new parent. Python discards the known rows and returns just one result. The service treats 1 < 500 as a complete search and never reaches the final parent. The cold one-node hierarchy then emits leaf 0 as a root with no parent, while the warm hierarchy emits an actual parent root. The adjacent 1,000- and 1,001-result controls pass; adding that one extra parent exposes the problem.

Please keep exclusions in the database with a parameter-safe representation, or page the scanned rows and explicitly track incomplete parent coverage. A short list after filtering a limited SQL page is not proof that all parents were found. Leaving this thread open.

_{Auto-replied by the GitHub Copilot app}

[Nimit Jain (Nimit3418)]

I have addressed this by replacing single-query chunk scanning with keyset-cursor pagination and returning database-level exhaustiveness (has_more) directly to the caller, removing the heuristic that treated a short filtered list as proof of completion.

1. Keyset cursor scanning across chunk pages:
   In _execute_get_scenario_parent_deltas_for_child_ids, when SQL exclusions are omitted (or partial), the query within each child chunk now loops using an ascending keyset cursor (AttackResultKeysetCursor(timestamp=..., id=...)) with scan_page_size = min(500, _MAX_BIND_VARS). If SQL returns a full page (len(rows) > scan_page_size) but Python filtering leaves len(unique_deltas) < limit, the chunk loop continues scanning past known rows until either limit is met or SQL returns < scan_page_size (proving that the database matches for that chunk are truly exhausted).

2. Explicit database completeness contract:
   Updated get_scenario_parent_deltas_for_child_ids_async to return tuple[list[ScenarioAttackResultDelta], bool] ((new_parents, has_more)), matching the bounded pagination contract of get_scenario_attack_result_deltas_async and get_scenario_recount_summaries_async.

3. Eliminated short-list termination heuristic:
   In scenario_run_service.py, replaced if len(new_parents) < page_limit: break with if not has_more: break. If the database indicates has_more but no new parents are found within the bounded scan, layer_clipped is explicitly set to True, ensuring unverified candidates are guarded and never falsely emitted as roots.

4. 1,002-result regression test:
   Added test_1002_results_500_leaves_501_parents_leaf_0_not_emitted_as_root_parity in test_scenario_hierarchy_parity.py matching your exact SQLite scenario (500 leaves, 501 parents referencing leaf 1 covering leaves 1..499, and 1 final parent referencing leaf 0). Verifies that the cold max_nodes=1 hierarchy does not emit leaf 0 as a root, correctly attaches leaf 0 under parent 501, and agrees with the warm hierarchy.

[Roman Lutz (romanlutz)]

Rechecked 1d14bd2. The keyset scan fixes the previous shared-parent example. There is still a false-root case when the result limit is reached partway through the final child-ID chunk's fetched rows (memory_interface.py:7292-7303). has_more_in_chunk checks for rows beyond the 500-row scan page, but not rows left unprocessed inside that page.

Here is a 1,001-result SQLite example. Save 500 leaves, then these 501 parents in timestamp order:

• 490 parents referencing leaves 0 through 489, one each.
• Two parents referencing leaf 490.
• Nine parents referencing leaves 491 through 499, one each.

SQLite splits the 500 child IDs into chunks of 490 and 10. The first chunk returns 490 parents. The final chunk fetches 11 parents, but the row loop accepts only 10 before reaching the result limit of 500. Both has_more conditions are false, so the last parent is silently omitted and the service marks the search complete. The cold max_nodes=1 hierarchy emits leaf 499 as a root with no parent, while the warm hierarchy emits an actual parent root. The adjacent 500-parent control passes; 502 and 510 parents also reproduce the problem.

Please include unprocessed rows from the current SQL page when deciding has_more. A final SQL page shorter than 500 rows is not proof that the result loop consumed that page. Leaving this thread open.

_{Auto-replied by the GitHub Copilot app}

[Nimit Jain (Nimit3418)]

---

I have cleanly merged `upstream/main` and resolved all conflicts across `scenario_progress_read_model.py`, `scenario_run_service.py`, `memory_interface.py`, and `test_azure_sql_memory.py`.

Beyond resolving text conflicts, I ensured full compatibility with recent changes from `main`:
1. **Sequential attack completions (#3118):** Aligned `_build_scenario_history_ranked_units` and group/run metric queries so that `completed` status is preserved and unit outcome statistics match `completed_units` identically.
2. **Post-run live delta polling (#3183):** Maintained keyset cursor polling for updated attempts without short-circuiting on terminal runs, while keeping cold hierarchy requests bounded.
3. **Diagnostic retry parity:** Ensured retry diagnostics capture attempts where `total_retries > 0`.

All 2,890 backend tests and all 72 hierarchy parity tests are passing locally with zero lint or formatting warnings.

[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.