From 292e4ff812bd2ffd905bf67d90929670d89e6163 Mon Sep 17 00:00:00 2001 From: pureliture Date: Mon, 22 Jun 2026 08:50:28 +0900 Subject: [PATCH] =?UTF-8?q?fix(vuln):=20PR=20#64=20=EB=A6=AC=EB=B7=B0=20?= =?UTF-8?q?=ED=9B=84=EC=86=8D=20=E2=80=94=20=EA=B2=A9=EB=A6=AC=20=EA=B0=95?= =?UTF-8?q?=ED=99=94=20+=20expanduser=20+=20=ED=83=80=EC=9E=85=ED=9E=8C?= =?UTF-8?q?=ED=8A=B8?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gemini-code-assist 리뷰 3건 반영. - runtime/vulnerability_scan.py(HIGH): _scan_one_catalog_repo가 SemgrepExecutionError 외 예외(clobber ValueError, import/SARIF 오류 등)를 잡지 못해 전체 카탈로그 루프가 중단될 수 있던 문제 수정. clobber 체크를 try 안으로 옮기고 일반 Exception을 per-repo scan-error로 격리("a single repo's failure never aborts the whole pass" 충족). - runtime/vulnerability_scan.py(MED): resolve_semgrep_binary가 ~ 경로를 처리하도록 Path(binary).expanduser() 적용. - cli/commands/vulnerability.py(MED): _append_freshness_notification result 인자에 VulnFreshnessResult 타입힌트 + import 추가. - tests: no-clobber/일반예외가 raise 대신 격리된 scan-error로 보고되는지 검증으로 갱신·추가. proof: uv run pytest 1356 passed/4 skipped; ruff clean. Co-Authored-By: Claude Opus 4.8 --- .gitignore | 5 ++ .../cli/commands/vulnerability.py | 5 +- .../runtime/vulnerability_scan.py | 19 ++++-- tests/test_vuln_catalog_scan.py | 64 +++++++++++++++---- 4 files changed, 76 insertions(+), 17 deletions(-) diff --git a/.gitignore b/.gitignore index 2301e46..aaa82a3 100644 --- a/.gitignore +++ b/.gitignore @@ -74,5 +74,10 @@ build/ !.claude/skills/** .routine-harness/ +# Multi-agent scratch (briefings/handoffs may contain private absolute paths) +.agents/* +!.agents/skills/ +!.agents/skills/** + # Local git worktrees .worktrees/ diff --git a/src/security_scanner/cli/commands/vulnerability.py b/src/security_scanner/cli/commands/vulnerability.py index 081ecf6..f34a0e4 100644 --- a/src/security_scanner/cli/commands/vulnerability.py +++ b/src/security_scanner/cli/commands/vulnerability.py @@ -19,6 +19,7 @@ from security_scanner.cli._store import dynamodb_config_from_args from security_scanner.runtime.vulnerability_freshness import ( VulnFreshnessRequest, + VulnFreshnessResult, run_vuln_freshness_eval, ) from security_scanner.runtime.vulnerability_scan import ( @@ -331,7 +332,9 @@ def cmd_vuln_freshness_eval(args: argparse.Namespace) -> int: return 0 -def _append_freshness_notification(log_path: str, result) -> None: +def _append_freshness_notification( + log_path: str, result: VulnFreshnessResult +) -> None: path = Path(log_path) path.parent.mkdir(parents=True, exist_ok=True) record = { diff --git a/src/security_scanner/runtime/vulnerability_scan.py b/src/security_scanner/runtime/vulnerability_scan.py index 67c26e2..91fc169 100644 --- a/src/security_scanner/runtime/vulnerability_scan.py +++ b/src/security_scanner/runtime/vulnerability_scan.py @@ -206,7 +206,7 @@ def resolve_semgrep_binary(binary: str) -> str | None: resolved = shutil.which(binary) if resolved is not None: return resolved - candidate = Path(binary) + candidate = Path(binary).expanduser() if candidate.is_absolute() and candidate.exists(): return str(candidate) return None @@ -312,12 +312,12 @@ def _scan_one_catalog_repo( artifact_path = ( artifact_root / _safe_repo_dir(entry.repo_id) / f"{request.scan_run_id}.jsonl" ) - if artifact_path.exists(): - raise ValueError( - f"refusing to clobber existing vuln artifact: {artifact_path}" - ) try: + if artifact_path.exists(): + raise ValueError( + f"refusing to clobber existing vuln artifact: {artifact_path}" + ) result = run_vulnerability_scan( ScanVulnerabilityRequest( root=cache_path, @@ -337,6 +337,15 @@ def _scan_one_catalog_repo( status=status, detail=_short_detail(exc), ) + except Exception as exc: # noqa: BLE001 - per-repo isolation: one repo's + # unexpected failure (clobber guard, import/SARIF errors, ...) must never + # abort the whole catalog pass; record it as a scan-error outcome. + return RepoScanOutcome( + repo_id=entry.repo_id, + repo_url=entry.repo_url, + status="scan-error", + detail=_short_detail(exc), + ) return RepoScanOutcome( repo_id=entry.repo_id, diff --git a/tests/test_vuln_catalog_scan.py b/tests/test_vuln_catalog_scan.py index cf6b357..c1badbd 100644 --- a/tests/test_vuln_catalog_scan.py +++ b/tests/test_vuln_catalog_scan.py @@ -10,8 +10,6 @@ import json from pathlib import Path -import pytest - from security_scanner.runtime.vulnerability_scan import ( CatalogScanRequest, run_catalog_vulnerability_scan, @@ -164,8 +162,10 @@ def test_two_runs_distinct_scan_run_ids_do_not_clobber(tmp_path): assert (artifact_dir / "alpha" / "run2.jsonl").is_file() -def test_reusing_same_scan_run_id_raises_no_clobber_guard(tmp_path): - store = FakeStore([_entry("alpha", included=True)]) +def test_reusing_same_scan_run_id_is_isolated_scan_error(tmp_path): + # The no-clobber guard must NOT abort the whole pass; a reused scan_run_id + # surfaces as an isolated per-repo scan-error outcome (PR #64 review HIGH). + store = FakeStore([_entry("alpha", included=True), _entry("beta", included=True)]) artifact_dir = tmp_path / "artifacts" fetcher = _make_fetcher(tmp_path) @@ -175,13 +175,20 @@ def test_reusing_same_scan_run_id_raises_no_clobber_guard(tmp_path): fetcher=fetcher, runner=FakeRunner(), ) - with pytest.raises(ValueError, match="clobber"): - run_catalog_vulnerability_scan( - CatalogScanRequest(artifact_dir=artifact_dir, scan_run_id="run1"), - store=store, - fetcher=fetcher, - runner=FakeRunner(), - ) + # Second pass with the same id: alpha would clobber; the pass still completes + # and reports the clobber as a scan-error rather than raising. + result = run_catalog_vulnerability_scan( + CatalogScanRequest(artifact_dir=artifact_dir, scan_run_id="run1"), + store=store, + fetcher=fetcher, + runner=FakeRunner(), + ) + + assert result.preflight_ok is True + by_repo = {o.repo_id: o for o in result.outcomes} + assert by_repo["alpha"].status == "scan-error" + assert "clobber" in (by_repo["alpha"].detail or "") + assert by_repo["beta"].status == "scan-error" def test_fetch_failure_for_one_repo_does_not_abort_run(tmp_path): @@ -241,3 +248,38 @@ def test_preflight_fails_without_runner_and_bogus_binary(tmp_path): assert "not found" in result.preflight_detail # No artifacts written when preflight fails. assert not artifact_dir.exists() + + +def test_unexpected_scan_exception_is_isolated_not_aborting(tmp_path): + # An unexpected (non-Semgrep) error in one repo's scan must be caught and + # reported as scan-error, never aborting the batch (PR #64 review HIGH). + store = FakeStore( + [_entry("alpha", included=True), _entry("beta", included=True)] + ) + artifact_dir = tmp_path / "artifacts" + + class FlakyRunner: + def __init__(self) -> None: + self.calls = 0 + + def run(self, root_path, *, sarif_path): + self.calls += 1 + if self.calls == 1: + raise RuntimeError("unexpected analyzer crash") + Path(sarif_path).write_text( + json.dumps(_sarif_payload()), encoding="utf-8" + ) + + result = run_catalog_vulnerability_scan( + CatalogScanRequest(artifact_dir=artifact_dir, scan_run_id="run1"), + store=store, + fetcher=_make_fetcher(tmp_path), + runner=FlakyRunner(), + ) + + by_repo = {o.repo_id: o for o in result.outcomes} + assert by_repo["alpha"].status == "scan-error" + assert "crash" in (by_repo["alpha"].detail or "") + assert by_repo["beta"].status == "scanned" + assert result.error_count == 1 + assert result.scanned_count == 1