Fix crescendo JSON parsing crash producing zero results (#5058399)#45526
Conversation
When Foundry's crescendo orchestration returns non-JSON responses, the ScenarioOrchestrator now catches the error gracefully instead of propagating it. Partial results from successful attack strategies (e.g., baseline) are preserved even when other strategies fail. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash that occurs when the Foundry crescendo orchestration returns non-JSON responses (e.g., raw markdown), causing run_async() to throw a JSON parsing exception and producing zero results. The fix wraps run_async() in a try/except inside ScenarioOrchestrator.execute() to catch such errors gracefully and attempt to recover any partial results from FoundryScenario._result.
Changes:
- Added a try/except around
self._scenario.run_async()inScenarioOrchestrator.execute()to catch exceptions and log a warning instead of propagating - Added a nested try/except to retrieve partial results from
self._scenario._resultif available after a failure
nagkumar91
left a comment
There was a problem hiding this comment.
This PR has significant issues that need to be addressed before merging.
1. Swallowing exceptions breaks the caller contract (critical)
_execution_manager.py wraps orchestrator.execute() in a try/except to catch failures and report "status": "failed" with error details. By catching and swallowing the exception here, the ExecutionManager never sees the failure — it reports the category as "status": "completed" with 0 results. Users will think the scan succeeded when it actually crashed. The exception should be re-raised after attempting partial recovery:
except Exception as e:
self.logger.warning(...)
try:
if self._scenario and hasattr(self._scenario, "_result"):
self._scenario_result = self._scenario._result
except Exception as recovery_error:
self.logger.debug(f"Failed to recover partial results: {recovery_error}")
raise # Let caller handle the failure2. Unvalidated partial result object
self._scenario._result is accessed without checking it has the expected .attack_results attribute. If the object is incomplete, get_attack_results() will later crash with AttributeError. Add validation:
candidate = self._scenario._result
if candidate and hasattr(candidate, "attack_results"):
self._scenario_result = candidate3. Silent inner except Exception: pass
Should at least log at debug level so recovery failures are diagnosable.
4. Overlap with PR #45541
Both PRs add partial-recovery logic at different layers. If both merge, errors get caught at two levels with potentially conflicting semantics. Please coordinate to decide on one layer for partial recovery.
5. Missing CHANGELOG entry and tests.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
0701e42 to
b1a7af2
Compare
nagkumar91
left a comment
There was a problem hiding this comment.
Re-reviewed — the previous issues have been addressed:
- ✅
hasattrguard on_resultaccess - ✅ Inner except now logs at debug level with
exc_info=True - ✅ Two new tests covering both partial-result and no-partial-result paths
- ✅ Clear comment explaining intentional exception swallowing and coordination with #45541
Note: This PR should be merged together with #45541 — if merged alone, failures would be silently treated as completions with 0 results since the exception is swallowed but no outer layer records the failure.
…45526) * Fix crescendo JSON parsing crash producing zero results (#5058399) When Foundry's crescendo orchestration returns non-JSON responses, the ScenarioOrchestrator now catches the error gracefully instead of propagating it. Partial results from successful attack strategies (e.g., baseline) are preserved even when other strategies fail. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review comments: remove redundant guard, add logging and docs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Apply black formatting Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…45526) * Fix crescendo JSON parsing crash producing zero results (#5058399) When Foundry's crescendo orchestration returns non-JSON responses, the ScenarioOrchestrator now catches the error gracefully instead of propagating it. Partial results from successful attack strategies (e.g., baseline) are preserved even when other strategies fail. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review comments: remove redundant guard, add logging and docs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Apply black formatting Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Bug Fix: #5058399
When Foundry crescendo orchestration returns non-JSON responses (e.g., raw markdown), the ScenarioOrchestrator now catches the error gracefully instead of propagating it. Partial results from successful attack strategies (e.g., baseline) are preserved even when other strategies fail.
Changes