diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 791671763..cae83db87 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,6 +31,8 @@ jobs: - run: pnpm lint - name: Service and React dashboard tests run: pnpm --filter @sentry/warden-service-api test && pnpm --filter @sentry/warden-service test && pnpm --filter warden-service-app test + - name: Warden sweep script tests + run: pnpm test:sweep-scripts - name: Test env: VITEST_JUNIT: test-results.junit.xml diff --git a/package.json b/package.json index a0a885b92..4cc20615f 100644 --- a/package.json +++ b/package.json @@ -11,7 +11,8 @@ "dev": "pnpm --filter @sentry/warden dev", "lint": "oxlint . && pnpm --filter warden-service-app lint:types", "lint:fix": "oxlint --fix . && pnpm --filter warden-service-app lint:types", - "test": "pnpm --filter @sentry/warden-service-api test && pnpm --filter @sentry/warden-service test && pnpm --filter warden-service-app test && pnpm --filter @sentry/warden test", + "test": "pnpm --filter @sentry/warden-service-api test && pnpm --filter @sentry/warden-service test && pnpm --filter warden-service-app test && pnpm --filter @sentry/warden test && pnpm test:sweep-scripts", + "test:sweep-scripts": "python3 -B -m unittest discover -s skills/warden-sweep/scripts -p '*_test.py'", "test:coverage": "pnpm --filter @sentry/warden test:coverage", "test:watch": "pnpm --filter @sentry/warden test:watch", "test:examples": "pnpm --filter @sentry/warden test:examples", diff --git a/skills/warden-sweep/scripts/create_issue.py b/skills/warden-sweep/scripts/create_issue.py index d1293c890..bc8c5971f 100644 --- a/skills/warden-sweep/scripts/create_issue.py +++ b/skills/warden-sweep/scripts/create_issue.py @@ -37,6 +37,60 @@ ) +DISPLAY_FIELDS = ("severity", "skill", "file", "startLine", "title") + + +def join_finding_details( + verified: list[dict[str, Any]], + all_findings: list[dict[str, Any]], +) -> list[dict[str, Any]]: + """Fill each verified entry's display fields from its full finding. + + verified.jsonl holds only verdict data (findingId, verdict, confidence); + severity, skill, file, title and line live in all-findings.jsonl. + Display fields the verified entry sets to a value win; null or empty + ones count as unset. Two different findings with one findingId raise, + since the join could not tell which one the entry means. + """ + by_id: dict[str, dict[str, Any]] = {} + for f in all_findings: + finding_id = f.get("findingId") + if not isinstance(finding_id, str) or not finding_id: + continue + prior = by_id.get(finding_id) + if prior is not None and prior != f: + raise ValueError( + f"duplicate findingId {finding_id!r} in all-findings.jsonl " + "with different details" + ) + by_id[finding_id] = f + joined: list[dict[str, Any]] = [] + for entry in verified: + finding_id = entry.get("findingId") + finding = by_id.get(finding_id, {}) if isinstance(finding_id, str) else {} + location = finding.get("location") + if not isinstance(location, dict): + location = {} + details = { + "severity": finding.get("severity"), + "skill": finding.get("skill"), + "file": finding.get("file") or location.get("path"), + "startLine": location.get("startLine"), + "title": finding.get("title"), + } + overrides = { + k: v for k, v in entry.items() + if not (k in DISPLAY_FIELDS and v in (None, "")) + } + joined.append({**{k: v for k, v in details.items() if v is not None}, **overrides}) + return joined + + +def table_cell(text: Any) -> str: + """Keep text inside one markdown table cell: escape pipes, flatten newlines.""" + return str(text).replace("|", "\\|").replace("\r", " ").replace("\n", " ") + + def build_issue_body( run_id: str, scan_index: list[dict[str, Any]], @@ -80,14 +134,25 @@ def build_issue_body( lines.append("") lines.append("| Severity | Skill | File | Title |") lines.append("|----------|-------|------|-------|") - for f in verified: + # Verify agents return verdict fields (references/verify-prompt.md), so take severity, skill, file, line and title from all-findings.jsonl + by_id = {f["findingId"]: f for f in all_findings if isinstance(f.get("findingId"), str)} + for v in verified: + finding_id = v.get("findingId") + f = {**(by_id.get(finding_id, {}) if isinstance(finding_id, str) else {}), **v} sev = severity_badge(f.get("severity", "info")) skill = f.get("skill", "") file_path = f.get("file", "") - start_line = f.get("startLine") + # extract_findings.py nests the line under "location" (extract_findings.py:93-96); + # a top-level startLine (older records, or a verified entry) takes precedence + location_obj = f.get("location") + start_line = f.get("startLine") or ( + location_obj.get("startLine") if isinstance(location_obj, dict) else None + ) location = f"{file_path}:{start_line}" if start_line else file_path title = f.get("title", "") - lines.append(f"| {sev} | {skill} | `{location}` | {title} |") + lines.append( + f"| {table_cell(sev)} | {table_cell(skill)} | `{table_cell(location)}` | {table_cell(title)} |" + ) lines.append("") if skills: @@ -167,6 +232,7 @@ def main() -> None: rejected = read_jsonl(os.path.join(data_dir, "rejected.jsonl")) files_scanned = sum(1 for e in scan_index if e.get("status") == "complete") + verified = join_finding_details(verified, all_findings) # Build issue title = f"Warden Sweep {run_id}: {len(verified)} findings across {files_scanned} files" diff --git a/skills/warden-sweep/scripts/create_issue_breaks_test.py b/skills/warden-sweep/scripts/create_issue_breaks_test.py new file mode 100644 index 000000000..c5647cbb6 --- /dev/null +++ b/skills/warden-sweep/scripts/create_issue_breaks_test.py @@ -0,0 +1,245 @@ +#!/usr/bin/env python3 +"""End-to-end test for create_issue.py. + +Runs the script as a subprocess against a sweep directory on disk, with a +fake `gh` on PATH that records the issue body instead of calling GitHub. +Temp files are removed when the test exits. + +Run: python3 skills/warden-sweep/scripts/create_issue_breaks_test.py +""" +from __future__ import annotations + +import json +import os +import re +import subprocess +import sys +import tempfile +import unittest +from typing import Any + +SCRIPTS_DIR = os.path.dirname(os.path.abspath(__file__)) +CREATE_ISSUE = os.path.join(SCRIPTS_DIR, "create_issue.py") + +FAKE_GH = f"""#!{sys.executable} +import json +import os +import sys + +args = sys.argv[1:] +if args[:2] == ["label", "create"]: + sys.exit(0) +if args[:2] == ["issue", "create"]: + with open(os.environ["FAKE_GH_LOG"], "w") as f: + json.dump({{ + "title": args[args.index("--title") + 1], + "body": args[args.index("--body") + 1], + }}, f) + print("https://github.com/example/repo/issues/42") + sys.exit(0) +sys.exit(2) +""" + +FINDING_ID = "sentry-security-83c8eadd" + +# Full finding data, as extract_findings.py writes it. +FINDING: dict[str, Any] = { + "findingId": FINDING_ID, + "file": "src/auth.ts", + "skill": "security-review", + "severity": "high", + "title": "Missing authz check on workflow connect", + "location": {"path": "src/auth.ts", "startLine": 12, "endLine": 14}, +} + +# Verdict-only data, as the verify phase writes it. +VERIFIED: dict[str, Any] = { + "findingId": FINDING_ID, + "verdict": "verified", + "confidence": "high", +} + +VERIFIED_HEADER = "| Severity | Skill | File | Title |" + + +def write_jsonl(path: str, rows: list[Any]) -> None: + with open(path, "w") as f: + for row in rows: + f.write(json.dumps(row) + "\n") + + +def verified_rows(body: str) -> list[str]: + """Return the data rows of the Verified Findings table.""" + lines = body.splitlines() + start = lines.index(VERIFIED_HEADER) + 2 + rows: list[str] = [] + for line in lines[start:]: + if not line.startswith("|"): + break + rows.append(line) + return rows + + +def unescaped_pipes(row: str) -> int: + return len(re.findall(r"(? tuple[subprocess.CompletedProcess[str], str | None]: + """Run create_issue.py on a fresh sweep dir. + + Returns the process result and the issue body, or None when gh was + never called (the script stopped before posting). + """ + with tempfile.TemporaryDirectory() as tmp: + sweep_dir = os.path.join(tmp, "sweep") + data_dir = os.path.join(sweep_dir, "data") + os.makedirs(data_dir) + bin_dir = os.path.join(tmp, "bin") + os.makedirs(bin_dir) + gh_path = os.path.join(bin_dir, "gh") + log_path = os.path.join(tmp, "gh-issue.json") + + with open(gh_path, "w") as f: + f.write(FAKE_GH) + os.chmod(gh_path, 0o755) + + with open(os.path.join(data_dir, "manifest.json"), "w") as f: + json.dump({"runId": "run-1"}, f) + write_jsonl( + os.path.join(data_dir, "scan-index.jsonl"), + [{"status": "complete", "skills": ["security-review"]}], + ) + write_jsonl(os.path.join(data_dir, "all-findings.jsonl"), all_findings) + write_jsonl(os.path.join(data_dir, "verified.jsonl"), verified) + write_jsonl(os.path.join(data_dir, "rejected.jsonl"), []) + + env = dict(os.environ) + env["PATH"] = bin_dir + os.pathsep + env.get("PATH", "") + env["FAKE_GH_LOG"] = log_path + + result = subprocess.run( + [sys.executable, CREATE_ISSUE, sweep_dir], + capture_output=True, + text=True, + env=env, + timeout=60, + ) + + body = None + if os.path.exists(log_path): + with open(log_path) as f: + body = json.load(f)["body"] + return result, body + + def assert_posted_rows(self, all_findings: list[Any], verified: list[Any], expected: list[str]) -> str: + result, body = self.run_sweep(all_findings, verified) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIsNotNone(body, "gh issue create was never called") + assert body is not None + self.assertEqual(verified_rows(body), expected) + return body + + def test_verified_rows_show_severity_skill_location_and_title(self) -> None: + result, body = self.run_sweep([FINDING], [VERIFIED]) + + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(json.loads(result.stdout)["issueNumber"], 42) + assert body is not None + self.assertIn( + "| **HIGH** | security-review | `src/auth.ts:12` | " + "Missing authz check on workflow connect |", + body, + ) + self.assertNotIn("| info | | `` | |", body) + + def test_pipe_in_title_does_not_add_a_table_cell(self) -> None: + finding = {**FINDING, "title": "Bad | title"} + body = self.assert_posted_rows( + [finding], + [VERIFIED], + ["| **HIGH** | security-review | `src/auth.ts:12` | Bad \\| title |"], + ) + self.assertEqual(unescaped_pipes(verified_rows(body)[0]), 5) + + def test_pipe_in_skill_does_not_add_a_table_cell(self) -> None: + finding = {**FINDING, "skill": "sec|skill"} + self.assert_posted_rows( + [finding], + [VERIFIED], + ["| **HIGH** | sec\\|skill | `src/auth.ts:12` | Missing authz check on workflow connect |"], + ) + + def test_forged_row_in_title_stays_inside_its_cell(self) -> None: + finding = { + **FINDING, + "title": "x | **CRITICAL** | forged | `src/evil.ts:1` | Forged finding", + } + self.assert_posted_rows( + [finding], + [VERIFIED], + [ + "| **HIGH** | security-review | `src/auth.ts:12` | " + "x \\| **CRITICAL** \\| forged \\| `src/evil.ts:1` \\| Forged finding |" + ], + ) + + def test_newline_in_title_keeps_row_on_one_line(self) -> None: + finding = {**FINDING, "title": "First line\nsecond line"} + self.assert_posted_rows( + [finding], + [VERIFIED], + ["| **HIGH** | security-review | `src/auth.ts:12` | First line second line |"], + ) + + def test_non_object_location_is_treated_as_missing(self) -> None: + for location in ["src/auth.ts:12", ["src/auth.ts", 12]]: + with self.subTest(location=location): + finding = {**FINDING, "location": location} + self.assert_posted_rows( + [finding], + [VERIFIED], + ["| **HIGH** | security-review | `src/auth.ts` | Missing authz check on workflow connect |"], + ) + + def test_unhashable_finding_id_does_not_crash_or_borrow_details(self) -> None: + other_finding = {**FINDING, "findingId": ["not", "a", "string"]} + other_verified = {"findingId": ["not", "a", "string"], "verdict": "verified", "confidence": "low"} + self.assert_posted_rows( + [FINDING, other_finding], + [VERIFIED, other_verified], + [ + "| **HIGH** | security-review | `src/auth.ts:12` | Missing authz check on workflow connect |", + "| info | | `` | |", + ], + ) + + def test_duplicate_finding_id_with_conflicting_details_fails_before_posting(self) -> None: + second = {**FINDING, "severity": "low", "title": "Other title", "location": {"path": "src/b.ts", "startLine": 2}} + for order in ([FINDING, second], [second, FINDING]): + with self.subTest(first=order[0]["title"]): + result, body = self.run_sweep(order, [VERIFIED]) + self.assertNotEqual(result.returncode, 0) + self.assertIn("duplicate findingId", result.stderr) + self.assertIsNone(body) + + def test_null_display_fields_in_verified_fall_back_to_finding(self) -> None: + verified = { + **VERIFIED, + "severity": None, + "file": "", + "title": None, + } + self.assert_posted_rows( + [FINDING], + [verified], + ["| **HIGH** | security-review | `src/auth.ts:12` | Missing authz check on workflow connect |"], + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/skills/warden-sweep/scripts/create_issue_test.py b/skills/warden-sweep/scripts/create_issue_test.py new file mode 100644 index 000000000..ef73f77fe --- /dev/null +++ b/skills/warden-sweep/scripts/create_issue_test.py @@ -0,0 +1,153 @@ +"""End-to-end tests for create_issue.py. + +Each test writes a warden log, runs the real extract_findings.py on it, lays +out a sweep directory from the extractor's output, then runs create_issue.py +as a subprocess. A stub `gh` on PATH records the issue it would create. +""" +import json +import os +import stat +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + + +SCRIPTS_DIR = Path(__file__).resolve().parent +CREATE_ISSUE = SCRIPTS_DIR / "create_issue.py" +EXTRACT_FINDINGS = SCRIPTS_DIR / "extract_findings.py" + +FAKE_GH = """#!{python} +import json, os, sys +args = sys.argv[1:] +if args[:2] == ["issue", "create"]: + with open(os.environ["FAKE_GH_LOG"], "w") as f: + json.dump(args, f) + print("https://github.com/getsentry/warden/issues/999") +""" + + +def run_sweep(findings: list[dict], verified: list[dict]) -> dict: + """Run create_issue.py on a sweep built from real extractor output. + + `findings` are raw warden finding objects (location nested as the agent writes them). + A verified entry may carry `"matchTitle"` instead of `findingId`; it is replaced by + the findingId the extractor assigned to the finding with that title. + """ + with tempfile.TemporaryDirectory() as tmp: + data_dir = os.path.join(tmp, "sweep", "data") + os.makedirs(data_dir) + bin_dir = os.path.join(tmp, "bin") + os.makedirs(bin_dir) + + log_path = os.path.join(tmp, "warden.jsonl") + with open(log_path, "w") as f: + f.write(json.dumps({ + "skill": "sentry-security", + "run": {"runId": "r1"}, + "findings": findings, + }) + "\n") + f.write(json.dumps({"type": "summary"}) + "\n") + + all_path = os.path.join(data_dir, "all-findings.jsonl") + subprocess.run( + [sys.executable, "-B", str(EXTRACT_FINDINGS), log_path, "-o", all_path], + check=True, capture_output=True, text=True, + ) + with open(all_path) as f: + extracted = [json.loads(line) for line in f if line.strip()] + id_by_title = {e["title"]: e["findingId"] for e in extracted} + + verified_rows = [] + for v in verified: + v = dict(v) + if "matchTitle" in v: + v["findingId"] = id_by_title[v.pop("matchTitle")] + verified_rows.append(v) + + with open(os.path.join(tmp, "sweep", "data", "manifest.json"), "w") as f: + json.dump({"runId": "r1"}, f) + with open(os.path.join(data_dir, "scan-index.jsonl"), "w") as f: + f.write(json.dumps({"status": "complete", "skills": ["sentry-security"]}) + "\n") + with open(os.path.join(data_dir, "verified.jsonl"), "w") as f: + f.writelines(json.dumps(v) + "\n" for v in verified_rows) + open(os.path.join(data_dir, "rejected.jsonl"), "w").close() + + gh_path = os.path.join(bin_dir, "gh") + with open(gh_path, "w") as f: + f.write(FAKE_GH.format(python=sys.executable)) + os.chmod(gh_path, os.stat(gh_path).st_mode | stat.S_IXUSR) + + gh_log = os.path.join(tmp, "gh-args.json") + env = { + **os.environ, + "PATH": bin_dir + os.pathsep + os.environ.get("PATH", ""), + "FAKE_GH_LOG": gh_log, + } + result = subprocess.run( + [sys.executable, "-B", str(CREATE_ISSUE), os.path.join(tmp, "sweep")], + env=env, capture_output=True, text=True, + ) + if result.returncode != 0: + raise AssertionError(f"create_issue.py failed: {result.stderr}") + + with open(gh_log) as f: + args = json.load(f) + with open(os.path.join(data_dir, "manifest.json")) as f: + manifest = json.load(f) + + return { + "title": args[args.index("--title") + 1], + "body": args[args.index("--body") + 1], + "issueUrl": manifest.get("issueUrl"), + } + + +class CreateIssueEndToEndTest(unittest.TestCase): + def test_verified_row_shows_file_and_line_from_extractor_record(self) -> None: + out = run_sweep( + findings=[ + { + "title": "SQL injection", + "severity": "high", + "confidence": "high", + "location": {"path": "src/a.ts", "startLine": 3, "endLine": 5}, + }, + { + "title": "Not verified", + "severity": "low", + "location": {"path": "src/b.ts", "startLine": 9}, + }, + ], + verified=[{"matchTitle": "SQL injection", "verdict": "verified", "confidence": "high"}], + ) + + self.assertIn("| **HIGH** | sentry-security | `src/a.ts:3` | SQL injection |", out["body"]) + self.assertNotIn("Not verified", out["body"]) + self.assertEqual(out["title"], "Warden Sweep r1: 1 findings across 1 files") + self.assertEqual(out["issueUrl"], "https://github.com/getsentry/warden/issues/999") + + def test_unmatched_verified_entry_keeps_its_row(self) -> None: + out = run_sweep( + findings=[{"title": "Unrelated", "severity": "low", "location": {"path": "c.py", "startLine": 1}}], + verified=[{"findingId": "missing", "verdict": "verified"}], + ) + + self.assertIn("| Verified | 1 |", out["body"]) + self.assertIn("| info | | `` | |", out["body"]) + + def test_verified_fields_override_all_findings(self) -> None: + out = run_sweep( + findings=[{"title": "old", "severity": "low", "location": {"path": "a.py", "startLine": 1}}], + verified=[{ + "matchTitle": "old", "verdict": "verified", "severity": "medium", + "skill": "s2", "file": "b.py", "startLine": 7, "title": "t2", + }], + ) + + self.assertIn("| MEDIUM | s2 | `b.py:7` | t2 |", out["body"]) + + +if __name__ == "__main__": + unittest.main()