From 7272700e649db0f42c5c880a62db28651c8a6b31 Mon Sep 17 00:00:00 2001 From: Terry <186405119+tlq5l@users.noreply.github.com> Date: Thu, 22 Oct 2026 06:14:17 +0700 Subject: [PATCH 1/6] fix(sweep): Fill the tracking issue findings table from all-findings Co-Authored-By: Claude Sonnet 5.5 Agent: claude --- skills/warden-sweep/scripts/create_issue.py | 5 +- .../warden-sweep/scripts/create_issue_test.py | 61 +++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 skills/warden-sweep/scripts/create_issue_test.py diff --git a/skills/warden-sweep/scripts/create_issue.py b/skills/warden-sweep/scripts/create_issue.py index d1293c890..bbee085df 100644 --- a/skills/warden-sweep/scripts/create_issue.py +++ b/skills/warden-sweep/scripts/create_issue.py @@ -80,7 +80,10 @@ def build_issue_body( lines.append("") lines.append("| Severity | Skill | File | Title |") lines.append("|----------|-------|------|-------|") - for f in verified: + # verified.jsonl holds verdict data only; take the details from all-findings.jsonl + by_id = {f["findingId"]: f for f in all_findings if "findingId" in f} + for v in verified: + f = {**by_id.get(v.get("findingId"), {}), **v} sev = severity_badge(f.get("severity", "info")) skill = f.get("skill", "") file_path = f.get("file", "") 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..a09c3b4ef --- /dev/null +++ b/skills/warden-sweep/scripts/create_issue_test.py @@ -0,0 +1,61 @@ +import importlib.util +import unittest +from pathlib import Path + + +SCRIPT_PATH = Path(__file__).with_name("create_issue.py") +SPEC = importlib.util.spec_from_file_location("create_issue", SCRIPT_PATH) +assert SPEC is not None +create_issue = importlib.util.module_from_spec(SPEC) +assert SPEC.loader is not None +SPEC.loader.exec_module(create_issue) + + +class BuildIssueBodyTest(unittest.TestCase): + def test_verified_rows_use_details_from_all_findings(self) -> None: + all_findings = [ + { + "findingId": "sec-1", + "severity": "high", + "skill": "sentry-security", + "file": "src/a.ts", + "title": "SQL injection", + "startLine": 3, + }, + { + "findingId": "sec-2", + "severity": "low", + "skill": "sentry-security", + "file": "src/b.ts", + "title": "Not verified", + }, + ] + verified = [{"findingId": "sec-1", "verdict": "verified", "confidence": "high"}] + + body = create_issue.build_issue_body("r1", [], all_findings, verified, []) + + self.assertIn("| **HIGH** | sentry-security | `src/a.ts:3` | SQL injection |", body) + self.assertNotIn("Not verified", body) + + def test_unmatched_verified_entry_keeps_its_row(self) -> None: + verified = [{"findingId": "missing", "verdict": "verified"}] + + body = create_issue.build_issue_body("r1", [], [], verified, []) + + self.assertIn("| Verified | 1 |", body) + self.assertIn("| info | | `` | |", body) + + def test_full_verified_entry_wins_over_all_findings(self) -> None: + all_findings = [{"findingId": "x", "severity": "low", "title": "old"}] + verified = [ + {"findingId": "x", "severity": "medium", "skill": "s2", "file": "b.py", + "title": "t2", "startLine": 7, "verdict": "verified"} + ] + + body = create_issue.build_issue_body("r1", [], all_findings, verified, []) + + self.assertIn("| MEDIUM | s2 | `b.py:7` | t2 |", body) + + +if __name__ == "__main__": + unittest.main() From 1e44121b705a1d7935a601dcf11dbcc76b3e95f7 Mon Sep 17 00:00:00 2001 From: Terry <186405119+tlq5l@users.noreply.github.com> Date: Wed, 4 Nov 2026 13:24:00 +0700 Subject: [PATCH 2/6] fix(sweep): Cite the verify prompt in the findings table comment Agent: claude --- skills/warden-sweep/scripts/create_issue.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skills/warden-sweep/scripts/create_issue.py b/skills/warden-sweep/scripts/create_issue.py index bbee085df..141302fad 100644 --- a/skills/warden-sweep/scripts/create_issue.py +++ b/skills/warden-sweep/scripts/create_issue.py @@ -80,7 +80,7 @@ def build_issue_body( lines.append("") lines.append("| Severity | Skill | File | Title |") lines.append("|----------|-------|------|-------|") - # verified.jsonl holds verdict data only; take the details from all-findings.jsonl + # Verify agents return verdict fields (references/verify-prompt.md), so take severity, skill, file and title from all-findings.jsonl by_id = {f["findingId"]: f for f in all_findings if "findingId" in f} for v in verified: f = {**by_id.get(v.get("findingId"), {}), **v} From db55b51a5c5b17117614ebcfe4cbbb8d5904d3f2 Mon Sep 17 00:00:00 2001 From: Terry <186405119+tlq5l@users.noreply.github.com> Date: Wed, 4 Nov 2026 15:46:04 +0700 Subject: [PATCH 3/6] fix(sweep): Render finding lines from the extractor's location create_issue.py read startLine at the top level, but extract_findings.py nests it under location, so every verified row lost its line number. The regression test now runs create_issue.py end to end on extractor output, and CI runs it. Agent: claude --- .github/workflows/ci.yml | 2 + package.json | 3 +- skills/warden-sweep/scripts/create_issue.py | 6 +- .../warden-sweep/scripts/create_issue_test.py | 176 +++++++++++++----- 4 files changed, 142 insertions(+), 45 deletions(-) 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 141302fad..08b41f1f1 100644 --- a/skills/warden-sweep/scripts/create_issue.py +++ b/skills/warden-sweep/scripts/create_issue.py @@ -80,14 +80,16 @@ def build_issue_body( lines.append("") lines.append("| Severity | Skill | File | Title |") lines.append("|----------|-------|------|-------|") - # Verify agents return verdict fields (references/verify-prompt.md), so take severity, skill, file and title from all-findings.jsonl + # 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 "findingId" in f} for v in verified: f = {**by_id.get(v.get("findingId"), {}), **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 + start_line = f.get("startLine") or (f.get("location") or {}).get("startLine") location = f"{file_path}:{start_line}" if start_line else file_path title = f.get("title", "") lines.append(f"| {sev} | {skill} | `{location}` | {title} |") diff --git a/skills/warden-sweep/scripts/create_issue_test.py b/skills/warden-sweep/scripts/create_issue_test.py index a09c3b4ef..ef73f77fe 100644 --- a/skills/warden-sweep/scripts/create_issue_test.py +++ b/skills/warden-sweep/scripts/create_issue_test.py @@ -1,60 +1,152 @@ -import importlib.util +"""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 -SCRIPT_PATH = Path(__file__).with_name("create_issue.py") -SPEC = importlib.util.spec_from_file_location("create_issue", SCRIPT_PATH) -assert SPEC is not None -create_issue = importlib.util.module_from_spec(SPEC) -assert SPEC.loader is not None -SPEC.loader.exec_module(create_issue) +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") +""" -class BuildIssueBodyTest(unittest.TestCase): - def test_verified_rows_use_details_from_all_findings(self) -> None: - all_findings = [ - { - "findingId": "sec-1", - "severity": "high", - "skill": "sentry-security", - "file": "src/a.ts", - "title": "SQL injection", - "startLine": 3, - }, - { - "findingId": "sec-2", - "severity": "low", + +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", - "file": "src/b.ts", - "title": "Not verified", - }, - ] - verified = [{"findingId": "sec-1", "verdict": "verified", "confidence": "high"}] + "run": {"runId": "r1"}, + "findings": findings, + }) + "\n") + f.write(json.dumps({"type": "summary"}) + "\n") - body = create_issue.build_issue_body("r1", [], all_findings, verified, []) + 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} - self.assertIn("| **HIGH** | sentry-security | `src/a.ts:3` | SQL injection |", body) - self.assertNotIn("Not verified", body) + 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) - def test_unmatched_verified_entry_keeps_its_row(self) -> None: - verified = [{"findingId": "missing", "verdict": "verified"}] + 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) - body = create_issue.build_issue_body("r1", [], [], verified, []) + 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}") - self.assertIn("| Verified | 1 |", body) - self.assertIn("| info | | `` | |", body) + 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"}], + ) - def test_full_verified_entry_wins_over_all_findings(self) -> None: - all_findings = [{"findingId": "x", "severity": "low", "title": "old"}] - verified = [ - {"findingId": "x", "severity": "medium", "skill": "s2", "file": "b.py", - "title": "t2", "startLine": 7, "verdict": "verified"} - ] + self.assertIn("| Verified | 1 |", out["body"]) + self.assertIn("| info | | `` | |", out["body"]) - body = create_issue.build_issue_body("r1", [], all_findings, verified, []) + 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 |", body) + self.assertIn("| MEDIUM | s2 | `b.py:7` | t2 |", out["body"]) if __name__ == "__main__": From d406cfaabad5b6ef434d0c4837843de364f6bd12 Mon Sep 17 00:00:00 2001 From: Terry <186405119+tlq5l@users.noreply.github.com> Date: Fri, 6 Nov 2026 18:43:30 +0700 Subject: [PATCH 4/6] fix(warden-sweep): Fill verified rows from full findings verified.jsonl only holds verdict data, so the tracking issue's findings table rendered empty severity, skill, file and title cells. Join each verified entry to its finding in all-findings.jsonl by findingId before rendering. Adds an end-to-end test that runs create_issue.py on a sweep directory with a fake gh and checks the rendered row. Agent: claude --- skills/warden-sweep/scripts/create_issue.py | 27 ++++ .../warden-sweep/scripts/test_create_issue.py | 118 ++++++++++++++++++ 2 files changed, 145 insertions(+) create mode 100644 skills/warden-sweep/scripts/test_create_issue.py diff --git a/skills/warden-sweep/scripts/create_issue.py b/skills/warden-sweep/scripts/create_issue.py index 08b41f1f1..f67db6eac 100644 --- a/skills/warden-sweep/scripts/create_issue.py +++ b/skills/warden-sweep/scripts/create_issue.py @@ -37,6 +37,32 @@ ) +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. + Fields the verified entry already sets win. + """ + by_id = {f["findingId"]: f for f in all_findings if f.get("findingId")} + joined: list[dict[str, Any]] = [] + for entry in verified: + finding = by_id.get(entry.get("findingId", ""), {}) + location = finding.get("location") or {} + 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"), + } + joined.append({**{k: v for k, v in details.items() if v is not None}, **entry}) + return joined + + def build_issue_body( run_id: str, scan_index: list[dict[str, Any]], @@ -172,6 +198,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/test_create_issue.py b/skills/warden-sweep/scripts/test_create_issue.py new file mode 100644 index 000000000..1b9066e2b --- /dev/null +++ b/skills/warden-sweep/scripts/test_create_issue.py @@ -0,0 +1,118 @@ +#!/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/test_create_issue.py +""" +from __future__ import annotations + +import json +import os +import subprocess +import sys +import tempfile +import unittest + +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) +""" + + +def write_jsonl(path: str, rows: list[dict]) -> None: + with open(path, "w") as f: + for row in rows: + f.write(json.dumps(row) + "\n") + + +class CreateIssueEndToEnd(unittest.TestCase): + def test_verified_rows_show_severity_skill_location_and_title(self) -> None: + 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"]}], + ) + # Full finding data, as extract_findings.py writes it. + write_jsonl( + os.path.join(data_dir, "all-findings.jsonl"), + [{ + "findingId": "sentry-security-83c8eadd", + "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. + write_jsonl( + os.path.join(data_dir, "verified.jsonl"), + [{ + "findingId": "sentry-security-83c8eadd", + "verdict": "verified", + "confidence": "high", + }], + ) + 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, + ) + + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(json.loads(result.stdout)["issueNumber"], 42) + with open(log_path) as f: + body = json.load(f)["body"] + + self.assertIn( + "| **HIGH** | security-review | `src/auth.ts:12` | " + "Missing authz check on workflow connect |", + body, + ) + self.assertNotIn("| info | | `` | |", body) + + +if __name__ == "__main__": + unittest.main() From 6326c66e02a02547028c7bf49b9137b6517ec8bb Mon Sep 17 00:00:00 2001 From: Terry <186405119+tlq5l@users.noreply.github.com> Date: Fri, 6 Nov 2026 21:54:00 +0700 Subject: [PATCH 5/6] fix(warden-sweep): Escape and validate joined finding details The verified-findings join pulls title, file and skill from all-findings.jsonl into the issue table. Those values are scanned from code, so: - escape pipes and flatten newlines in table cells, so a title or skill cannot add cells or forge a row - treat a non-object location as missing instead of crashing - ignore non-string findingIds instead of crashing on unhashable values - fail before posting when one findingId maps to two different findings, rather than letting file order pick the row - null or empty display fields in verified.jsonl no longer override the finding's values Each case has an end-to-end regression test that failed before this fix. Agent: claude --- skills/warden-sweep/scripts/create_issue.py | 42 +++- .../warden-sweep/scripts/test_create_issue.py | 191 +++++++++++++++--- 2 files changed, 195 insertions(+), 38 deletions(-) diff --git a/skills/warden-sweep/scripts/create_issue.py b/skills/warden-sweep/scripts/create_issue.py index f67db6eac..2dde10424 100644 --- a/skills/warden-sweep/scripts/create_issue.py +++ b/skills/warden-sweep/scripts/create_issue.py @@ -37,6 +37,9 @@ ) +DISPLAY_FIELDS = ("severity", "skill", "file", "startLine", "title") + + def join_finding_details( verified: list[dict[str, Any]], all_findings: list[dict[str, Any]], @@ -45,13 +48,29 @@ def join_finding_details( verified.jsonl holds only verdict data (findingId, verdict, confidence); severity, skill, file, title and line live in all-findings.jsonl. - Fields the verified entry already sets win. + 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 = {f["findingId"]: f for f in all_findings if f.get("findingId")} + 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 = by_id.get(entry.get("findingId", ""), {}) - location = finding.get("location") or {} + 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"), @@ -59,10 +78,19 @@ def join_finding_details( "startLine": location.get("startLine"), "title": finding.get("title"), } - joined.append({**{k: v for k, v in details.items() if v is not None}, **entry}) + 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]], @@ -118,7 +146,9 @@ def build_issue_body( start_line = f.get("startLine") or (f.get("location") or {}).get("startLine") 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: diff --git a/skills/warden-sweep/scripts/test_create_issue.py b/skills/warden-sweep/scripts/test_create_issue.py index 1b9066e2b..38f5ac033 100644 --- a/skills/warden-sweep/scripts/test_create_issue.py +++ b/skills/warden-sweep/scripts/test_create_issue.py @@ -11,10 +11,12 @@ 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") @@ -38,15 +40,61 @@ sys.exit(2) """ +FINDING_ID = "sentry-security-83c8eadd" -def write_jsonl(path: str, rows: list[dict]) -> None: +# 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"(? None: + def run_sweep( + self, + all_findings: list[Any], + verified: list[Any], + ) -> 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") @@ -66,27 +114,8 @@ def test_verified_rows_show_severity_skill_location_and_title(self) -> None: os.path.join(data_dir, "scan-index.jsonl"), [{"status": "complete", "skills": ["security-review"]}], ) - # Full finding data, as extract_findings.py writes it. - write_jsonl( - os.path.join(data_dir, "all-findings.jsonl"), - [{ - "findingId": "sentry-security-83c8eadd", - "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. - write_jsonl( - os.path.join(data_dir, "verified.jsonl"), - [{ - "findingId": "sentry-security-83c8eadd", - "verdict": "verified", - "confidence": "high", - }], - ) + 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) @@ -101,17 +130,115 @@ def test_verified_rows_show_severity_skill_location_and_title(self) -> None: timeout=60, ) - self.assertEqual(result.returncode, 0, result.stderr) - self.assertEqual(json.loads(result.stdout)["issueNumber"], 42) - with open(log_path) as f: - body = json.load(f)["body"] + 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 - self.assertIn( + 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` | " - "Missing authz check on workflow connect |", - body, - ) - self.assertNotIn("| info | | `` | |", body) + "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__": From d78271f622ab151ce688a623a7caa9bd792e69b4 Mon Sep 17 00:00:00 2001 From: Terry <186405119+tlq5l@users.noreply.github.com> Date: Sat, 7 Nov 2026 12:37:22 +0700 Subject: [PATCH 6/6] fix(warden-sweep): Guard issue table rows against odd location and id values build_issue_body read startLine from a location that may be a string or list, and keyed findings by findingId without checking the type, so one malformed finding crashed the tracking issue. Treat a non-object location as missing and skip non-string findingIds, as join_finding_details does. Move the end-to-end tests to create_issue_breaks_test.py so the sweep test command (*_test.py) runs them. Agent: claude --- skills/warden-sweep/scripts/create_issue.py | 10 +++++++--- ...est_create_issue.py => create_issue_breaks_test.py} | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) rename skills/warden-sweep/scripts/{test_create_issue.py => create_issue_breaks_test.py} (99%) diff --git a/skills/warden-sweep/scripts/create_issue.py b/skills/warden-sweep/scripts/create_issue.py index 2dde10424..bc8c5971f 100644 --- a/skills/warden-sweep/scripts/create_issue.py +++ b/skills/warden-sweep/scripts/create_issue.py @@ -135,15 +135,19 @@ def build_issue_body( lines.append("| Severity | Skill | File | Title |") lines.append("|----------|-------|------|-------|") # 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 "findingId" in f} + by_id = {f["findingId"]: f for f in all_findings if isinstance(f.get("findingId"), str)} for v in verified: - f = {**by_id.get(v.get("findingId"), {}), **v} + 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", "") # 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 - start_line = f.get("startLine") or (f.get("location") or {}).get("startLine") + 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( diff --git a/skills/warden-sweep/scripts/test_create_issue.py b/skills/warden-sweep/scripts/create_issue_breaks_test.py similarity index 99% rename from skills/warden-sweep/scripts/test_create_issue.py rename to skills/warden-sweep/scripts/create_issue_breaks_test.py index 38f5ac033..c5647cbb6 100644 --- a/skills/warden-sweep/scripts/test_create_issue.py +++ b/skills/warden-sweep/scripts/create_issue_breaks_test.py @@ -5,7 +5,7 @@ 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/test_create_issue.py +Run: python3 skills/warden-sweep/scripts/create_issue_breaks_test.py """ from __future__ import annotations