Skip to content

fix(report): propagate artifact write errors in finish_scan (#1108) #1117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AyushDubey23 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(report): propagate artifact write errors in finish_scan (#1108) #1117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 42 additions & 4 deletions strix/report/state.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,11 @@ def add_vulnerability_report(
if self.vulnerability_found_callback:
self.vulnerability_found_callback(report)

self.save_run_data()
try:
self.save_run_data()
except Exception:
self.vulnerability_reports.pop()
raise
return report_id

def get_existing_vulnerabilities(self) -> list[dict[str, Any]]:
Expand Down Expand Up @@ -371,6 +375,10 @@ def update_scan_final_fields(
technical_analysis: str,
recommendations: str,
) -> None:
prev_scan_results = self.scan_results
prev_final_scan_result = self.final_scan_result
prev_run_scan_results = self.run_record.get("scan_results")

self.scan_results = {
"scan_completed": True,
"executive_summary": executive_summary.strip(),
Expand All @@ -384,7 +392,17 @@ def update_scan_final_fields(
self.run_record["scan_results"] = self.scan_results

logger.info("Updated scan final fields")
self.save_run_data(mark_complete=True)
try:
self.save_run_data(mark_complete=True)
except Exception:
self.scan_results = prev_scan_results
self.final_scan_result = prev_final_scan_result
if prev_run_scan_results is None:
self.run_record.pop("scan_results", None)
else:
self.run_record["scan_results"] = prev_run_scan_results
raise

posthog.end(self, exit_reason="finished_by_tool")
scarf.end(self, exit_reason="finished_by_tool")

Expand All @@ -410,6 +428,9 @@ def set_scan_config(self, config: dict[str, Any]) -> None:
)

def save_run_data(self, mark_complete: bool = False, status: str | None = None) -> None:
prev_end_time = self.end_time
prev_status = self.run_record.get("status")

if mark_complete:
self.end_time = datetime.now(UTC).isoformat()
self.run_record["end_time"] = self.end_time
Expand All @@ -424,10 +445,26 @@ def save_run_data(self, mark_complete: bool = False, status: str | None = None)
self.run_record["status"] = status

self._sync_llm_usage_record()
self._save_artifacts()
try:
self._save_artifacts()
except Exception:
if mark_complete or status:
self.end_time = prev_end_time
if prev_end_time is None:
self.run_record.pop("end_time", None)
else:
self.run_record["end_time"] = prev_end_time
if prev_status is not None:
self.run_record["status"] = prev_status
else:
self.run_record.pop("status", None)
raise

def cleanup(self, status: str = "stopped") -> None:
self.save_run_data(status=status)
try:
self.save_run_data(status=status)
except (OSError, RuntimeError):
logger.exception("Failed to save scan data during cleanup")

def _format_final_scan_result(self, scan_results: dict[str, Any]) -> str:
return f"""# Executive Summary
Expand Down Expand Up @@ -480,6 +517,7 @@ def _save_artifacts(self) -> None:
logger.info("Essential scan data saved to: %s", run_dir)
except (OSError, RuntimeError):
logger.exception("Failed to save scan data")
raise

@SebTardif SebTardif Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Re-raising from _save_artifacts is the right finish_scan fix, but every save_run_data() caller now surfaces write errors. add_vulnerability_report appends to self.vulnerability_reports and then calls save_run_data() with no rollback. create_vulnerability_report / create_dependency_report still catch only (ImportError, AttributeError), so a mid-scan disk-full or permission error becomes an unhandled tool exception after the report is already in memory. A retry then hits duplicate detection against a finding that never landed on disk.

Suggestion: Either catch (OSError, RuntimeError) in those reporting tools and return a structured success: false (same pattern as _do_finish), or roll back the in-memory append if save_run_data() fails. record_sdk_usage is already wrapped in except Exception in hooks, so that path is fine.

Comment on lines 517 to +520

@greptile-apps greptile-apps Bot Aug 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Failed finish leaves final report

When write_executive_report succeeds but a later vulnerability or run-record write fails, this re-raise rolls back the in-memory final state without removing penetration_test_report.md. Cleanup cannot rewrite that file after final_scan_result is restored to None, so the viewer can load a completed report even though finish_scan returned failure and the run is stopped.

Knowledge Base Used:

Prompt To Fix With AI 
This is a comment left during a code review.
Path: strix/report/state.py
Line: 517-520

Comment:
**Failed finish leaves final report**

When `write_executive_report` succeeds but a later vulnerability or run-record write fails, this re-raise rolls back the in-memory final state without removing `penetration_test_report.md`. Cleanup cannot rewrite that file after `final_scan_result` is restored to `None`, so the viewer can load a completed report even though `finish_scan` returned failure and the run is stopped.

**Knowledge Base Used:**
- [Reporting and Output](https://app.greptile.com/strix-org-3/-/custom-context/knowledge-base/usestrix/strix/-/docs/reporting-and-output.md)
- [Tools Overview](https://app.greptile.com/strix-org-3/-/custom-context/knowledge-base/usestrix/strix/-/docs/tools-overview.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.


def _sarif_repository_context(self) -> dict[str, Any] | None:
"""Repo/commit/branch context for SARIF provenance (repo scans only).
Expand Down
2 changes: 1 addition & 1 deletion strix/tools/finish/tool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def _do_finish(
recommendations=recommendations.strip(),
)
vuln_count = len(report_state.vulnerability_reports)
except (ImportError, AttributeError) as e:
except (ImportError, AttributeError, OSError, RuntimeError) as e:
logger.exception("finish_scan persistence failed")
return {"success": False, "error": f"Failed to complete scan: {e!s}"}
Comment on lines +66 to 68

@greptile-apps greptile-apps Bot Aug 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Failed finish retains completed state

When the first final artifact write fails but a later cleanup write succeeds, save_run_data(mark_complete=True) has already marked the in-memory run record completed and this error path does not restore it. Cleanup then preserves and persists that completed status even though finish_scan returned failure and the coordinator was never marked completed.

Knowledge Base Used:

Prompt To Fix With AI 
This is a comment left during a code review.
Path: strix/tools/finish/tool.py
Line: 66-68

Comment:
**Failed finish retains completed state**

When the first final artifact write fails but a later cleanup write succeeds, `save_run_data(mark_complete=True)` has already marked the in-memory run record completed and this error path does not restore it. Cleanup then preserves and persists that completed status even though `finish_scan` returned failure and the coordinator was never marked completed.

**Knowledge Base Used:**
- [Reporting and Output](https://app.greptile.com/strix-org-3/-/custom-context/knowledge-base/usestrix/strix/-/docs/reporting-and-output.md)
- [Tools Overview](https://app.greptile.com/strix-org-3/-/custom-context/knowledge-base/usestrix/strix/-/docs/tools-overview.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

else:
Expand Down
4 changes: 2 additions & 2 deletions strix/tools/reporting/tool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ async def _do_create( # noqa: PLR0912
agent_id=agent_id if isinstance(agent_id, str) else None,
agent_name=agent_name if isinstance(agent_name, str) else None,
)
except (ImportError, AttributeError) as e:
except (ImportError, AttributeError, OSError, RuntimeError) as e:
logger.exception("create_vulnerability_report persistence failed")
return {"success": False, "error": f"Failed to create vulnerability report: {e!s}"}
else:
Expand Down Expand Up @@ -1088,7 +1088,7 @@ async def _do_create_dependency( # noqa: PLR0912
agent_id=agent_id if isinstance(agent_id, str) else None,
agent_name=agent_name if isinstance(agent_name, str) else None,
)
except (ImportError, AttributeError) as e:
except (ImportError, AttributeError, OSError, RuntimeError) as e:
logger.exception("create_dependency_report persistence failed")
return {"success": False, "error": f"Failed to create dependency report: {e!s}"}
else:
Expand Down
102 changes: 101 additions & 1 deletion tests/test_reporting_fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
check_duplicate,
)
from strix.report.state import ReportState, set_global_report_state
from strix.tools.finish.tool import finish_scan
from strix.tools.finish.tool import _do_finish, finish_scan
from strix.tools.reporting.tool import (
_do_create,
_do_create_dependency,
Expand Down Expand Up @@ -1087,3 +1087,103 @@ async def test_dependency_report_rejects_contextual_breakdown_without_reasoning(
assert result["success"] is False
assert any("contextual_cvss_reasoning is required" in error for error in result["errors"])
assert report_state.vulnerability_reports == []


def test_save_artifacts_propagates_write_error(
report_state: ReportState, monkeypatch: pytest.MonkeyPatch
) -> None:
def _failing_write(*_args: object, **_kwargs: object) -> None:
raise OSError("Disk quota exceeded")

monkeypatch.setattr("strix.report.state.write_executive_report", _failing_write)
report_state.final_scan_result = "Test report"

with pytest.raises(OSError, match="Disk quota exceeded"):
report_state.save_run_data()


@pytest.mark.asyncio
async def test_finish_scan_reports_failure_on_artifact_write_error(
report_state: ReportState, monkeypatch: pytest.MonkeyPatch

@SebTardif SebTardif Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] report_state is required so the fixture installs the global state that _do_finish reads, but the test body never references it. Ruff ARG001 flags this (make lint / make check-all). A naive unused-arg cleanup that drops or renames the parameter would stop injecting the fixture and the test would hit leftover global state instead of the isolated one.

Suggestion: Use report_state in an assertion (for example assert report_state.scan_results is None after the failed finish), matching the rollback test below.

) -> None:
def _failing_write(*_args: object, **_kwargs: object) -> None:
raise OSError("Permission denied")

monkeypatch.setattr("strix.report.state.write_executive_report", _failing_write)

result = _do_finish(
parent_id=None,
executive_summary="Summary",
methodology="Methodology",
technical_analysis="Analysis",
recommendations="Recommendations",
)

assert result["success"] is False
assert "Failed to complete scan: Permission denied" in result["error"]
assert "scan_completed" not in result or result.get("scan_completed") is not True
assert report_state.scan_results is None


@pytest.mark.asyncio
async def test_failed_finish_rolls_back_in_memory_completion_state(
report_state: ReportState, monkeypatch: pytest.MonkeyPatch
) -> None:
def _failing_write(*_args: object, **_kwargs: object) -> None:
raise OSError("Permission denied")

monkeypatch.setattr("strix.report.state.write_executive_report", _failing_write)

result = _do_finish(
parent_id=None,
executive_summary="Summary",
methodology="Methodology",
technical_analysis="Analysis",
recommendations="Recommendations",
)

assert result["success"] is False
assert report_state.scan_results is None
assert report_state.final_scan_result is None
assert report_state.run_record.get("status") == "running"

monkeypatch.undo()
report_state.cleanup(status="stopped")
assert report_state.run_record.get("status") == "stopped"
assert report_state.run_record.get("scan_results") is None


@pytest.mark.asyncio
async def test_create_vulnerability_report_reports_failure_on_write_error(
report_state: ReportState, monkeypatch: pytest.MonkeyPatch
) -> None:
def _failing_write(*_args: object, **_kwargs: object) -> None:
raise OSError("Read-only file system")

monkeypatch.setattr("strix.report.state.write_vulnerabilities", _failing_write)

result = await _do_create(
title="SQL Injection in Login",
description="Exploitable SQL injection in authentication form.",
impact="Full database access.",
target="http://example.com/login",
technical_analysis="Unsanitized user input passed to query.",
poc_description="Submit ' OR '1'='1 payload.",
poc_script_code="curl http://example.com/login",
remediation_steps="Use parameterized queries.",
evidence="HTTP 500 SQL error trace",
assumptions="Target DB is PostgreSQL",
fix_effort="low",
cvss_breakdown=_CVSS,
endpoint="/login",
method="POST",
cve=None,
cwe="CWE-89",
code_locations=None,
fix_pr_body=None,
)

assert result["success"] is False
assert "Failed to create vulnerability report: Read-only file system" in result["error"]
assert report_state.vulnerability_reports == []