From 603e9014932d8ec754d63f846b310029b39a0eb4 Mon Sep 17 00:00:00 2001 From: rgarcia <72655+rgarcia@users.noreply.github.com> Date: Thu, 5 Nov 2026 21:20:59 +0000 Subject: [PATCH 1/3] Move Semgrep triage from Claude Code to Pi --- .github/workflows/semgrep-tests.yml | 29 +++ .github/workflows/semgrep-triage-prompt.md | 72 ++++---- .github/workflows/semgrep.yml | 59 ++++++- README.md | 6 +- scripts/semgrep_triage.py | 163 +++++++++++++++++ scripts/test_semgrep_triage.py | 196 +++++++++++++++++++++ 6 files changed, 481 insertions(+), 44 deletions(-) create mode 100644 .github/workflows/semgrep-tests.yml create mode 100644 scripts/semgrep_triage.py create mode 100644 scripts/test_semgrep_triage.py diff --git a/.github/workflows/semgrep-tests.yml b/.github/workflows/semgrep-tests.yml new file mode 100644 index 0000000..4f14d6e --- /dev/null +++ b/.github/workflows/semgrep-tests.yml @@ -0,0 +1,29 @@ +name: Semgrep Tests + +on: + pull_request: + paths: + - .github/workflows/semgrep.yml + - .github/workflows/semgrep-tests.yml + - scripts/semgrep_triage.py + - scripts/test_semgrep_triage.py + push: + branches: [main] + paths: + - .github/workflows/semgrep.yml + - .github/workflows/semgrep-tests.yml + - scripts/semgrep_triage.py + - scripts/test_semgrep_triage.py + +permissions: + contents: read + +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + - run: | + python3 -m venv /tmp/semgrep-tests + /tmp/semgrep-tests/bin/pip install PyYAML==6.0.3 + /tmp/semgrep-tests/bin/python -m unittest discover -s scripts -p test_semgrep_triage.py diff --git a/.github/workflows/semgrep-triage-prompt.md b/.github/workflows/semgrep-triage-prompt.md index 48a95f1..628d6f8 100644 --- a/.github/workflows/semgrep-triage-prompt.md +++ b/.github/workflows/semgrep-triage-prompt.md @@ -1,32 +1,27 @@ You are a security analyst triaging Semgrep static analysis findings for a pull request. -The GitHub CLI is available as `gh` and authenticated via GH_TOKEN. You can comment on pull requests but must not create or edit PRs directly. +You have read-only file tools and no shell or GitHub access. Do not post anything yourself: +your final response is the result, and the workflow posts it to the PR. # Context -- Repo: ${GITHUB_REPOSITORY} +- Repo: ${GITHUB_REPOSITORY} (the PR checkout is the current directory) - Codebase: ${CODEBASE_DESCRIPTION} # Task -1. Get the PR number and commit SHA: - ``` - PR_NUMBER=$(gh pr view --json number --jq '.number') - COMMIT_SHA=$(git rev-parse HEAD) - ``` +1. Read semgrep-results.json -2. Read semgrep-results.json +2. For each finding, read the affected source file and surrounding code to understand context -3. For each finding, read the affected source file and surrounding code to understand context - -4. Evaluate each finding: +3. Evaluate each finding: - Is this a true positive with real security impact? - Is the vulnerable code path reachable from user input? - Are there existing sanitizations or mitigations nearby? -5. If NO valuable findings: output "No actionable security findings." and exit without posting a comment +4. If NO valuable findings: respond with `{"comments": [], "omitted": 0}` -6. If valuable findings exist: post inline review comments on the PR +5. If valuable findings exist: respond with inline review comments for them (see Output) # Triage Rules @@ -43,20 +38,22 @@ Only report findings that are: - True positives in production code paths - Not already mitigated by upstream sanitization -# Comment Format (only if valuable findings exist) +# Output -Post inline review comments on the exact lines where issues were found using the GitHub API: +Respond with only this JSON object, with no other text: ``` -gh api repos/${GITHUB_REPOSITORY}/pulls/$PR_NUMBER/reviews \ - --method POST \ - -f commit_id=$COMMIT_SHA \ - -f event=COMMENT \ - -f 'comments[][path]=' \ - -F 'comments[][line]=' \ - -f 'comments[][body]=' +{ + "comments": [ + {"path": "", "line": , "body": ""} + ], + "omitted": +} ``` +`path` is the file path as it appears in semgrep-results.json, and `line` is the flagged line +in the PR version of the file. List comments highest severity first. + Each inline comment body should include: 1. What the issue is and why it is a true positive 2. Recommended fix with a code snippet if applicable @@ -67,28 +64,31 @@ Each inline comment body should include: - Add path to .semgrepignore to exclude the file entirely - Both options should be mentioned with the exact rule ID from the finding -# Deduplication (strictly enforced) +# Commentable Lines -Before posting anything, fetch ALL existing inline review comments on this PR: +Review comments can only be placed on lines inside the PR diff. ${TRIAGE_DIR}/commentable-lines.json +maps each changed file to the `[first, last]` line ranges of its diff hunks. Do not comment on a +line outside those ranges. -``` -gh api repos/${GITHUB_REPOSITORY}/pulls/$PR_NUMBER/comments -``` +# Deduplication (strictly enforced) + +${TRIAGE_DIR}/existing-comments.json lists ALL existing inline review comments on this PR, with +their file, line, original line, and author. For each Semgrep finding, check whether ANY existing comment (from any author -- github-actions[bot], cursor[bot], vercel[bot], socket-security[bot], or any human) already covers the same file and line. If a comment already exists on that line, -skip it entirely -- do NOT post a duplicate regardless of whether the existing +skip it entirely -- do NOT include a duplicate regardless of whether the existing comment is from us or another bot. -Only post a comment for a finding if that exact file+line has zero existing comments. +Only include a comment for a finding if that exact file+line has zero existing comments. # Constraints -- Do NOT post any comments if there are no actionable findings -- Do NOT post if an existing comment already covers the finding (any author) -- Post inline review comments, not top-level PR comments -- Post all new findings in a single review API call (batch the comments[] array) -- Post at most 5 inline comments -- if more than 5 actionable findings remain after - triage, post only the 5 highest severity ones and note how many were omitted -- Do not create or edit PRs directly +- Do NOT include any comments if there are no actionable findings +- Do NOT include a comment if an existing comment already covers the finding (any author) +- Comments are inline review comments on the flagged line, never top-level PR comments +- All new findings are posted in a single review +- Include at most 5 comments -- if more than 5 actionable findings remain after + triage, include only the 5 highest severity ones and set `omitted` to how many were left out +- Do not create or edit files diff --git a/.github/workflows/semgrep.yml b/.github/workflows/semgrep.yml index 3ce93b6..eaa0d7f 100644 --- a/.github/workflows/semgrep.yml +++ b/.github/workflows/semgrep.yml @@ -91,24 +91,71 @@ jobs: timeout-minutes: 15 needs: scan if: needs.scan.outputs.findings_count != '0' && fromJson(needs.scan.outputs.findings_count) <= 20 + # The agent gets read-only file tools and no GitHub token. Runner steps gather the PR's + # existing comments before it runs and post its validated result as one review after. + env: + REPO: ${{ github.repository }} + NUMBER: ${{ github.event.pull_request.number }} steps: - name: Checkout repository uses: actions/checkout@v4 + with: + persist-credentials: false - name: Download results uses: actions/download-artifact@v4 with: name: semgrep-results - - name: Install Claude Code CLI + - name: Fetch triage prompt and script + run: | + echo "TRIAGE_DIR=$RUNNER_TEMP/semgrep-triage" >> "$GITHUB_ENV" + base=https://raw.githubusercontent.com/kernel/security-workflows/main + curl -fsSL "$base/.github/workflows/semgrep-triage-prompt.md" -o "$RUNNER_TEMP/semgrep-triage-prompt.md" + curl -fsSL "$base/scripts/semgrep_triage.py" -o "$RUNNER_TEMP/semgrep_triage.py" + + - name: Gather PR comments + env: + GH_TOKEN: ${{ github.token }} + run: python3 "$RUNNER_TEMP/semgrep_triage.py" gather --repo "$REPO" --number "$NUMBER" --dir "$TRIAGE_DIR" + + - name: Setup Node + uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 + with: + node-version: '22' + + - name: Install Pi + env: + PI_TELEMETRY: '0' run: | - curl -fsSL https://claude.ai/install.sh | bash - echo "$HOME/.local/bin" >> $GITHUB_PATH + npm install -g --ignore-scripts @earendil-works/pi-coding-agent + pi --version - name: Triage findings env: ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} + PI_PREFERRED_MODEL: ${{ vars.PI_PREFERRED_MODEL }} + PI_SKIP_VERSION_CHECK: '1' + PI_TELEMETRY: '0' + CODEBASE_DESCRIPTION: ${{ inputs.codebase-description }} + run: | + : "${PI_PREFERRED_MODEL:?Set the PI_PREFERRED_MODEL organization variable}" + # shellcheck disable=SC2016 # envsubst takes the variable names literally + envsubst '${GITHUB_REPOSITORY} ${CODEBASE_DESCRIPTION} ${TRIAGE_DIR}' \ + < "$RUNNER_TEMP/semgrep-triage-prompt.md" > "$TRIAGE_DIR/prompt.md" + # The checkout is PR content: --no-approve keeps its .pi resources from loading and + # --no-context-files keeps its AGENTS.md and CLAUDE.md out of the instructions. + pi --print --no-session --no-approve --no-context-files \ + --tools read,grep,find,ls \ + --model "$PI_PREFERRED_MODEL" --thinking max \ + @"$TRIAGE_DIR/prompt.md" > "$TRIAGE_DIR/agent-output.txt" + cat "$TRIAGE_DIR/agent-output.txt" + + - name: Post review + env: + GH_TOKEN: ${{ github.token }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: | - export CODEBASE_DESCRIPTION="${{ inputs.codebase-description }}" - curl -fsSL https://raw.githubusercontent.com/kernel/security-workflows/main/.github/workflows/semgrep-triage-prompt.md | envsubst '${GITHUB_REPOSITORY} ${CODEBASE_DESCRIPTION}' | claude -p --model ${{ vars.CLAUDE_CODE_PREFERRED_MODEL }} --dangerously-skip-permissions --output-format text --verbose + python3 "$RUNNER_TEMP/semgrep_triage.py" post --repo "$REPO" --number "$NUMBER" \ + --commit "$HEAD_SHA" --dir "$TRIAGE_DIR" --results semgrep-results.json diff --git a/README.md b/README.md index c03e971..42077c1 100644 --- a/README.md +++ b/README.md @@ -131,7 +131,8 @@ projectIgnorePaths: Consumer repos need these secrets (set at org or repo level): -- `ANTHROPIC_API_KEY` — for the `semgrep.yml` triage agent and the `pr-risk.yml` classifier (Claude Code) +- `ANTHROPIC_API_KEY` — for the `semgrep.yml` triage agent (Pi) and the `pr-risk.yml` classifier (Claude Code) +- `OPENAI_API_KEY` — for the `semgrep.yml` triage agent (Pi), when `PI_PREFERRED_MODEL` is an OpenAI model - `CURSOR_API_KEY` — for the other fix/remediation agents (Cursor) - `ADMIN_APP_ID` + `ADMIN_APP_PRIVATE_KEY` — GitHub App for write access - `SOCKET_API_TOKEN` — Socket.dev API token @@ -140,5 +141,6 @@ Consumer repos need these secrets (set at org or repo level): Consumer repos need these variables (set at org or repo level): -- `CLAUDE_CODE_PREFERRED_MODEL` — model for the `semgrep.yml` triage agent and the `pr-risk.yml` classifier (Claude Code) +- `PI_PREFERRED_MODEL` — model for the `semgrep.yml` triage agent (Pi) +- `CLAUDE_CODE_PREFERRED_MODEL` — model for the `pr-risk.yml` classifier (Claude Code) - `CURSOR_PREFERRED_MODEL` — model for the other Cursor agent invocations diff --git a/scripts/semgrep_triage.py b/scripts/semgrep_triage.py new file mode 100644 index 0000000..96aabf4 --- /dev/null +++ b/scripts/semgrep_triage.py @@ -0,0 +1,163 @@ +#!/usr/bin/env python3 +"""GitHub side of the Semgrep triage agent, run by semgrep.yml. + +`gather` writes the PR's commentable lines and existing inline comments for the agent to read. +`post` checks the agent's JSON result against the Semgrep findings, the PR diff, and the +comments already on the PR, then posts the remaining comments as one review. + +The agent itself has no GitHub token or shell; only these runner steps call the API. +Needs only the standard library and the runner's `gh`. +""" + +from __future__ import annotations + +import argparse +import json +import re +import subprocess +import sys +from pathlib import Path +from typing import Any + + +MAX_COMMENTS = 5 + + +class GitHubError(Exception): + pass + + +class GitHub: + """The few API calls this needs, through the runner's `gh`, which reads GH_TOKEN.""" + + def __init__(self, repo: str) -> None: + self.repo = repo + + def api(self, *args: str, body: Any = None) -> str: + cmd = ["gh", "api", *args] + if body is not None: + cmd += ["--input", "-"] + result = subprocess.run(cmd, input=json.dumps(body) if body is not None else None, + capture_output=True, text=True) + if result.returncode: + raise GitHubError(result.stderr.strip()) + return result.stdout + + def paginate(self, path: str) -> list[Any]: + pages = json.loads(self.api("--paginate", "--slurp", path)) + return [item for page in pages for item in page] + + def files(self, number: int) -> list[dict[str, Any]]: + return self.paginate(f"/repos/{self.repo}/pulls/{number}/files?per_page=100") + + def review_comments(self, number: int) -> list[dict[str, Any]]: + return self.paginate(f"/repos/{self.repo}/pulls/{number}/comments?per_page=100") + + def create_review(self, number: int, review: dict[str, Any]) -> None: + self.api("--method", "POST", f"/repos/{self.repo}/pulls/{number}/reviews", body=review) + + +def commentable_lines(files: list[dict[str, Any]]) -> dict[str, list[list[int]]]: + """New-file line ranges of each diff hunk: the lines a review comment can be placed on.""" + lines: dict[str, list[list[int]]] = {} + for f in files: + for m in re.finditer(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@", f.get("patch") or "", re.M): + start, length = int(m.group(1)), int(m.group(2) or 1) + if length: + lines.setdefault(f["filename"], []).append([start, start + length - 1]) + return lines + + +def comment_locations(comments: list[dict[str, Any]]) -> list[dict[str, Any]]: + return [{"path": c["path"], "line": c.get("line"), "original_line": c.get("original_line"), + "author": (c.get("user") or {}).get("login")} for c in comments] + + +def gather(gh: GitHub, number: int, out: Path) -> None: + out.mkdir(parents=True, exist_ok=True) + (out / "commentable-lines.json").write_text(json.dumps(commentable_lines(gh.files(number)), indent=1)) + (out / "existing-comments.json").write_text(json.dumps(comment_locations(gh.review_comments(number)), indent=1)) + + +def parse_result(text: str) -> dict[str, Any]: + """The agent's final message: one JSON object, possibly inside a code fence.""" + text = text.strip() + fenced = re.fullmatch(r"```(?:json)?\s*\n(.*)\n```", text, re.S) + result = json.loads(fenced.group(1) if fenced else text) + if not isinstance(result, dict) or not isinstance(result.get("comments"), list): + raise ValueError("expected an object with a comments array") + omitted = result.setdefault("omitted", 0) + if not isinstance(omitted, int) or isinstance(omitted, bool) or omitted < 0: + raise ValueError("omitted must be a non-negative integer") + return result + + +def select_comments(result: dict[str, Any], findings: list[dict[str, Any]], + commentable: dict[str, list[list[int]]], + existing: list[dict[str, Any]]) -> tuple[list[dict[str, Any]], int]: + """Comments that sit on a Semgrep finding, inside the diff, on a line nobody has commented on.""" + taken = {(c["path"], n) for c in existing for n in (c.get("line"), c.get("original_line")) if n} + kept: list[dict[str, Any]] = [] + for c in result["comments"]: + path, line, body = (c.get("path"), c.get("line"), c.get("body")) if isinstance(c, dict) else (None,) * 3 + if not (isinstance(path, str) and isinstance(line, int) and not isinstance(line, bool) + and isinstance(body, str) and body.strip()): + print(f"::warning::Dropping malformed comment: {json.dumps(c)[:200]}") + continue + where = f"{path}:{line}" + if not any(f["path"] == path and f["start"]["line"] <= line <= f["end"]["line"] for f in findings): + print(f"::warning::Dropping comment on {where}: no Semgrep finding there") + elif not any(lo <= line <= hi for lo, hi in commentable.get(path, [])): + print(f"::warning::Dropping comment on {where}: line is outside the PR diff") + elif (path, line) in taken: + print(f"Skipping {where}: the line already has a review comment") + else: + taken.add((path, line)) + kept.append({"path": path, "line": line, "body": body}) + return kept[:MAX_COMMENTS], result["omitted"] + max(0, len(kept) - MAX_COMMENTS) + + +def post(gh: GitHub, number: int, commit: str, workdir: Path, results: Path) -> int: + try: + result = parse_result((workdir / "agent-output.txt").read_text()) + except (OSError, ValueError) as e: + print(f"::error::The triage agent did not return a valid result: {e}") + return 1 + findings = json.loads(results.read_text()).get("results", []) + commentable = json.loads((workdir / "commentable-lines.json").read_text()) + # Re-read right before posting: other reviewers may have commented while the agent ran. + existing = comment_locations(gh.review_comments(number)) + comments, omitted = select_comments(result, findings, commentable, existing) + if not comments: + print("No actionable security findings.") + return 0 + review: dict[str, Any] = {"commit_id": commit, "event": "COMMENT", "comments": comments} + if omitted: + review["body"] = (f"{omitted} more actionable Semgrep finding{'s' if omitted != 1 else ''} " + f"not posted (limit: {MAX_COMMENTS} inline comments).") + gh.create_review(number, review) + print(f"Posted {len(comments)} inline comment(s).") + return 0 + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser() + sub = parser.add_subparsers(dest="command", required=True) + for name in ("gather", "post"): + p = sub.add_parser(name) + p.add_argument("--repo", required=True) + p.add_argument("--number", type=int, required=True) + p.add_argument("--dir", type=Path, required=True) + if name == "post": + p.add_argument("--commit", required=True) + p.add_argument("--results", type=Path, required=True) + args = parser.parse_args(argv) + gh = GitHub(args.repo) + if args.command == "gather": + gather(gh, args.number, args.dir) + return 0 + return post(gh, args.number, args.commit, args.dir, args.results) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/test_semgrep_triage.py b/scripts/test_semgrep_triage.py new file mode 100644 index 0000000..ada2fc1 --- /dev/null +++ b/scripts/test_semgrep_triage.py @@ -0,0 +1,196 @@ +import io +import json +from contextlib import redirect_stdout +from pathlib import Path +import sys +import tempfile +import unittest + +import yaml + +sys.path.insert(0, str(Path(__file__).resolve().parent)) +import semgrep_triage # noqa: E402 + + +WORKFLOW = Path(__file__).resolve().parents[1] / ".github/workflows/semgrep.yml" + +FINDINGS = [ + {"path": "api/handler.go", "start": {"line": 40}, "end": {"line": 42}}, + {"path": "api/exec.go", "start": {"line": 7}, "end": {"line": 7}}, +] +COMMENTABLE = {"api/handler.go": [[30, 50]], "api/exec.go": [[1, 20]]} + + +def comment(path="api/handler.go", line=41, body="Injection risk."): + return {"path": path, "line": line, "body": body} + + +def select(comments, omitted=0, existing=()): + with redirect_stdout(io.StringIO()): + return semgrep_triage.select_comments({"comments": comments, "omitted": omitted}, + FINDINGS, COMMENTABLE, list(existing)) + + +class FakeGitHub: + def __init__(self, files=(), comments=()): + self._files = list(files) + self._comments = list(comments) + self.reviews = [] + + def files(self, number): + return self._files + + def review_comments(self, number): + return self._comments + + def create_review(self, number, review): + self.reviews.append((number, review)) + + +class CommentableLinesTest(unittest.TestCase): + def test_hunk_ranges_on_the_new_side(self): + files = [ + {"filename": "a.go", "patch": "@@ -1,3 +1,4 @@\n x\n+y\n@@ -20 +21,2 @@ func\n+z"}, + {"filename": "deleted.go", "patch": "@@ -1,2 +0,0 @@\n-a\n-b"}, + {"filename": "binary.png"}, + ] + self.assertEqual(semgrep_triage.commentable_lines(files), {"a.go": [[1, 4], [21, 22]]}) + + +class GatherTest(unittest.TestCase): + def test_writes_commentable_lines_and_existing_comment_locations(self): + gh = FakeGitHub(files=[{"filename": "a.go", "patch": "@@ -1 +1,2 @@\n+x"}], + comments=[{"path": "a.go", "line": 2, "original_line": 2, + "user": {"login": "cursor[bot]"}, "body": "not passed on"}]) + with tempfile.TemporaryDirectory() as tmp: + out = Path(tmp) / "triage" + semgrep_triage.gather(gh, 7, out) + self.assertEqual(json.loads((out / "commentable-lines.json").read_text()), {"a.go": [[1, 2]]}) + self.assertEqual(json.loads((out / "existing-comments.json").read_text()), + [{"path": "a.go", "line": 2, "original_line": 2, "author": "cursor[bot]"}]) + + +class ParseResultTest(unittest.TestCase): + def test_plain_and_fenced_json(self): + for text in ('{"comments": []}', '```json\n{"comments": [], "omitted": 2}\n```\n'): + self.assertEqual(semgrep_triage.parse_result(text)["comments"], []) + + def test_rejects_prose_and_bad_shapes(self): + for text in ("No actionable security findings.", '{"comments": {}}', "[]", + '{"comments": [], "omitted": -1}', '{"comments": [], "omitted": true}'): + with self.assertRaises(ValueError, msg=text): + semgrep_triage.parse_result(text) + + +class SelectCommentsTest(unittest.TestCase): + def test_keeps_comments_on_findings_inside_the_diff(self): + self.assertEqual(select([comment(), comment("api/exec.go", 7)]), + ([comment(), comment("api/exec.go", 7)], 0)) + + def test_drops_comments_off_a_finding_or_outside_the_diff(self): + kept, _ = select([comment(line=43), comment("api/other.go", 41), comment("api/exec.go", 7)]) + self.assertEqual(kept, [comment("api/exec.go", 7)]) + commentable = {"api/handler.go": [[30, 40]]} + with redirect_stdout(io.StringIO()): + kept, _ = semgrep_triage.select_comments({"comments": [comment()], "omitted": 0}, + FINDINGS, commentable, []) + self.assertEqual(kept, []) + + def test_skips_lines_that_already_have_a_comment_from_anyone(self): + existing = [ + {"path": "api/handler.go", "line": 41, "original_line": 41, "author": "cursor[bot]"}, + {"path": "api/exec.go", "line": None, "original_line": 7, "author": "someone"}, + ] + self.assertEqual(select([comment(), comment("api/exec.go", 7)], existing=existing), ([], 0)) + + def test_posts_one_comment_per_line(self): + self.assertEqual(select([comment(), comment(body="Again.")]), ([comment()], 0)) + + def test_drops_malformed_comments(self): + bad = [None, {"path": "api/handler.go", "line": "41", "body": "x"}, + {"path": "api/handler.go", "line": True, "body": "x"}, + {"path": "api/handler.go", "line": 41, "body": " "}] + self.assertEqual(select(bad), ([], 0)) + + def test_caps_comments_and_counts_the_rest_as_omitted(self): + comments = [comment(line=n) for n in (40, 41, 42)] + [comment("api/exec.go", 7)] + findings = FINDINGS + [{"path": "api/handler.go", "start": {"line": 30}, "end": {"line": 35}}] + with redirect_stdout(io.StringIO()): + kept, omitted = semgrep_triage.select_comments( + {"comments": comments + [comment(line=n) for n in (30, 31)], "omitted": 3}, + findings, COMMENTABLE, []) + self.assertEqual(kept, (comments + [comment(line=30)])[:semgrep_triage.MAX_COMMENTS]) + self.assertEqual(omitted, 4) + + +class PostTest(unittest.TestCase): + def run_post(self, output, existing=()): + gh = FakeGitHub(comments=existing) + with tempfile.TemporaryDirectory() as tmp: + workdir = Path(tmp) + (workdir / "agent-output.txt").write_text(output) + (workdir / "commentable-lines.json").write_text(json.dumps(COMMENTABLE)) + results = workdir / "semgrep-results.json" + results.write_text(json.dumps({"results": FINDINGS})) + out = io.StringIO() + with redirect_stdout(out): + code = semgrep_triage.post(gh, 7, "headsha", workdir, results) + return code, gh.reviews, out.getvalue() + + def test_posts_one_comment_review_on_the_head_commit(self): + code, reviews, _ = self.run_post(json.dumps({"comments": [comment()], "omitted": 0})) + self.assertEqual(code, 0) + self.assertEqual(reviews, [(7, {"commit_id": "headsha", "event": "COMMENT", "comments": [comment()]})]) + + def test_notes_omitted_findings_in_the_review_body(self): + _, reviews, _ = self.run_post(json.dumps({"comments": [comment()], "omitted": 2})) + self.assertIn("2 more actionable Semgrep findings", reviews[0][1]["body"]) + + def test_rechecks_existing_comments_before_posting(self): + existing = [{"path": "api/handler.go", "line": 41, "original_line": 41, "user": {"login": "x"}}] + code, reviews, out = self.run_post(json.dumps({"comments": [comment()]}), existing) + self.assertEqual((code, reviews), (0, [])) + self.assertIn("No actionable security findings.", out) + + def test_fails_without_a_valid_result(self): + code, reviews, out = self.run_post("I could not finish.") + self.assertEqual((code, reviews), (1, [])) + self.assertIn("::error::", out) + + +class WorkflowTest(unittest.TestCase): + def setUp(self): + workflow = yaml.safe_load(WORKFLOW.read_text()) + self.steps = workflow["jobs"]["triage"]["steps"] + self.names = [s.get("name") for s in self.steps] + + def step(self, name): + return self.steps[self.names.index(name)] + + def test_agent_step_has_no_github_token_or_write_tools(self): + agent = self.step("Triage findings") + text = json.dumps(agent) + self.assertNotIn("GH_TOKEN", text) + self.assertNotIn("GITHUB_TOKEN", text) + self.assertNotIn("github.token", text) + self.assertIn("--tools read,grep,find,ls ", agent["run"]) + self.assertIn("--no-approve", agent["run"]) + self.assertNotIn("--approve ", agent["run"]) + + def test_checkout_does_not_persist_the_token(self): + checkout = self.step("Checkout repository") + self.assertIs(checkout["with"]["persist-credentials"], False) + + def test_only_runner_steps_get_the_token_and_model_keys_stay_in_the_agent_step(self): + for step in self.steps: + env = step.get("env", {}) + if "GH_TOKEN" in env: + self.assertIn(step["name"], ("Gather PR comments", "Post review")) + if any("secrets." in str(v) for v in env.values()): + self.assertEqual(step["name"], "Triage findings") + self.assertLess(self.names.index("Gather PR comments"), self.names.index("Triage findings")) + self.assertLess(self.names.index("Triage findings"), self.names.index("Post review")) + + +if __name__ == "__main__": + unittest.main() From 75835da124a6bac3ad666961efec2817a2877039 Mon Sep 17 00:00:00 2001 From: rgarcia <72655+rgarcia@users.noreply.github.com> Date: Thu, 5 Nov 2026 21:49:01 +0000 Subject: [PATCH 2/3] Move Semgrep findings onto the PR head before triage --- .github/workflows/semgrep.yml | 12 +++-- scripts/semgrep_triage.py | 66 +++++++++++++++++++++--- scripts/test_semgrep_triage.py | 91 ++++++++++++++++++++++++++++++---- 3 files changed, 150 insertions(+), 19 deletions(-) diff --git a/.github/workflows/semgrep.yml b/.github/workflows/semgrep.yml index eaa0d7f..24b46f8 100644 --- a/.github/workflows/semgrep.yml +++ b/.github/workflows/semgrep.yml @@ -96,10 +96,14 @@ jobs: env: REPO: ${{ github.repository }} NUMBER: ${{ github.event.pull_request.number }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} steps: + # Semgrep scanned the merge commit, but review comments go on the PR head. Depth 2 fetches + # both, so the findings can be moved onto the head before the agent reads it. - name: Checkout repository uses: actions/checkout@v4 with: + fetch-depth: 2 persist-credentials: false - name: Download results @@ -117,7 +121,10 @@ jobs: - name: Gather PR comments env: GH_TOKEN: ${{ github.token }} - run: python3 "$RUNNER_TEMP/semgrep_triage.py" gather --repo "$REPO" --number "$NUMBER" --dir "$TRIAGE_DIR" + run: | + python3 "$RUNNER_TEMP/semgrep_triage.py" gather --repo "$REPO" --number "$NUMBER" \ + --head "$HEAD_SHA" --dir "$TRIAGE_DIR" --results semgrep-results.json + git checkout -q --detach "$HEAD_SHA" - name: Setup Node uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 @@ -155,7 +162,6 @@ jobs: - name: Post review env: GH_TOKEN: ${{ github.token }} - HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: | python3 "$RUNNER_TEMP/semgrep_triage.py" post --repo "$REPO" --number "$NUMBER" \ - --commit "$HEAD_SHA" --dir "$TRIAGE_DIR" --results semgrep-results.json + --head "$HEAD_SHA" --dir "$TRIAGE_DIR" --results semgrep-results.json diff --git a/scripts/semgrep_triage.py b/scripts/semgrep_triage.py index 96aabf4..22973b3 100644 --- a/scripts/semgrep_triage.py +++ b/scripts/semgrep_triage.py @@ -1,7 +1,9 @@ #!/usr/bin/env python3 """GitHub side of the Semgrep triage agent, run by semgrep.yml. -`gather` writes the PR's commentable lines and existing inline comments for the agent to read. +`gather` moves the Semgrep findings from the merge commit Semgrep scanned onto the PR head, which +is what review comments are placed on, and writes the PR's commentable lines and existing inline +comments for the agent to read. `post` checks the agent's JSON result against the Semgrep findings, the PR diff, and the comments already on the PR, then posts the remaining comments as one review. @@ -68,13 +70,64 @@ def commentable_lines(files: list[dict[str, Any]]) -> dict[str, list[list[int]]] return lines +HUNK_RE = re.compile(r"^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@") + + +def diff_hunks(diff: str) -> dict[str, list[tuple[int, int, int, int]]]: + """(old start, old length, new start, new length) of each hunk in a `git diff -U0`, by new path.""" + hunks: dict[str, list[tuple[int, int, int, int]]] = {} + path = None + for line in diff.splitlines(): + if line.startswith("+++ "): + path = line[6:] if line.startswith("+++ b/") else None + elif path and (m := HUNK_RE.match(line)): + a, b, c, d = m.groups() + hunks.setdefault(path, []).append((int(a), int(b or 1), int(c), int(d or 1))) + return hunks + + +def old_line(line: int, hunks: list[tuple[int, int, int, int]]) -> int | None: + """Where a line of the diff's new side sits on its old side; None if the diff added it.""" + shift = 0 + for a, b, c, d in hunks: + if d and c <= line < c + d: + return None + if (c + d - 1 if d else c) < line: + shift += b - d + return line + shift + + +def findings_on_head(results: dict[str, Any], head: str) -> dict[str, Any]: + """Move findings from the merge commit in the working tree onto the PR head. + + The base branch can change the same files as the PR, so a line number in the merge commit + can differ from the head's. Findings on lines that only the base branch added are dropped, + since the PR does not touch them and they cannot be commented on. + """ + paths = sorted({f["path"] for f in results.get("results", [])}) + diff = subprocess.run(["git", "-c", "core.quotePath=false", "diff", "-U0", "--no-renames", + "--no-ext-diff", "--no-textconv", head, "HEAD", "--", *paths], + capture_output=True, text=True, check=True).stdout + hunks = diff_hunks(diff) + kept = [] + for f in results.get("results", []): + start = old_line(f["start"]["line"], hunks.get(f["path"], [])) + if start is None: + continue + end = old_line(f["end"]["line"], hunks.get(f["path"], [])) or start + kept.append({**f, "start": {**f["start"], "line": start}, + "end": {**f["end"], "line": max(start, end)}}) + return {**results, "results": kept} + + def comment_locations(comments: list[dict[str, Any]]) -> list[dict[str, Any]]: return [{"path": c["path"], "line": c.get("line"), "original_line": c.get("original_line"), "author": (c.get("user") or {}).get("login")} for c in comments] -def gather(gh: GitHub, number: int, out: Path) -> None: +def gather(gh: GitHub, number: int, out: Path, results: Path, head: str) -> None: out.mkdir(parents=True, exist_ok=True) + results.write_text(json.dumps(findings_on_head(json.loads(results.read_text()), head))) (out / "commentable-lines.json").write_text(json.dumps(commentable_lines(gh.files(number)), indent=1)) (out / "existing-comments.json").write_text(json.dumps(comment_locations(gh.review_comments(number)), indent=1)) @@ -148,15 +201,14 @@ def main(argv: list[str] | None = None) -> int: p.add_argument("--repo", required=True) p.add_argument("--number", type=int, required=True) p.add_argument("--dir", type=Path, required=True) - if name == "post": - p.add_argument("--commit", required=True) - p.add_argument("--results", type=Path, required=True) + p.add_argument("--results", type=Path, required=True) + p.add_argument("--head", required=True) args = parser.parse_args(argv) gh = GitHub(args.repo) if args.command == "gather": - gather(gh, args.number, args.dir) + gather(gh, args.number, args.dir, args.results, args.head) return 0 - return post(gh, args.number, args.commit, args.dir, args.results) + return post(gh, args.number, args.head, args.dir, args.results) if __name__ == "__main__": diff --git a/scripts/test_semgrep_triage.py b/scripts/test_semgrep_triage.py index ada2fc1..cc9ca29 100644 --- a/scripts/test_semgrep_triage.py +++ b/scripts/test_semgrep_triage.py @@ -1,7 +1,9 @@ import io import json +import os from contextlib import redirect_stdout from pathlib import Path +import subprocess import sys import tempfile import unittest @@ -57,17 +59,79 @@ def test_hunk_ranges_on_the_new_side(self): self.assertEqual(semgrep_triage.commentable_lines(files), {"a.go": [[1, 4], [21, 22]]}) +def git(cwd, *args): + return subprocess.run(["git", *args], cwd=cwd, check=True, capture_output=True, text=True).stdout.strip() + + +class FindingsOnHeadTest(unittest.TestCase): + def test_old_line(self): + # Base added 3 lines at 2, replaced 2 lines at 20 with 1, and deleted 2 lines after 30. + hunks = [(1, 0, 2, 3), (17, 2, 20, 1), (28, 2, 30, 0)] + self.assertEqual(semgrep_triage.old_line(1, hunks), 1) + self.assertIsNone(semgrep_triage.old_line(3, hunks)) + self.assertEqual(semgrep_triage.old_line(10, hunks), 7) + self.assertIsNone(semgrep_triage.old_line(20, hunks)) + self.assertEqual(semgrep_triage.old_line(25, hunks), 23) + self.assertEqual(semgrep_triage.old_line(30, hunks), 28) + self.assertEqual(semgrep_triage.old_line(31, hunks), 31) + + class GatherTest(unittest.TestCase): - def test_writes_commentable_lines_and_existing_comment_locations(self): - gh = FakeGitHub(files=[{"filename": "a.go", "patch": "@@ -1 +1,2 @@\n+x"}], - comments=[{"path": "a.go", "line": 2, "original_line": 2, + def setUp(self): + tmp = tempfile.TemporaryDirectory() + self.addCleanup(tmp.cleanup) + self.root = Path(tmp.name) / "repo" + self.root.mkdir() + git(self.root, "init", "-q", "-b", "main") + git(self.root, "config", "user.email", "test@example.com") + git(self.root, "config", "user.name", "Test") + self.write(["def ping(host):", " return host", "", "", "def other():", " pass"]) + git(self.root, "add", "-A") + git(self.root, "commit", "-qm", "base") + git(self.root, "switch", "-qc", "pr") + self.write(["def ping(host):", " return run(host, shell=True)", "", "", "def other():", " pass"]) + git(self.root, "commit", "-qam", "pr") + self.head = git(self.root, "rev-parse", "HEAD") + git(self.root, "switch", "-q", "main") + self.write(["import subprocess", "", "", "def ping(host):", " return host", "", "", "def other():", + " subprocess.run(other, shell=True)"]) + git(self.root, "commit", "-qam", "main moved on") + # GitHub's refs/pull/N/merge: base as first parent, PR head as second. + git(self.root, "merge", "-q", "--no-ff", "-m", "merge", "pr") + self.assertEqual(git(self.root, "rev-parse", "HEAD^2"), self.head) + + def write(self, lines): + (self.root / "app.py").write_text("\n".join(lines) + "\n") + + def finding(self, line): + return {"path": "app.py", "check_id": "shell", + "start": {"line": line, "col": 5}, "end": {"line": line, "col": 9}} + + def test_moves_findings_onto_the_pr_head_and_writes_pr_context(self): + merge_lines = (self.root / "app.py").read_text().splitlines() + self.assertEqual(merge_lines[4], " return run(host, shell=True)") + results = self.root / "semgrep-results.json" + # Line 5 is the PR's change; line 9 only exists because the base branch added it. + results.write_text(json.dumps({"results": [self.finding(5), self.finding(9)], "errors": []})) + patch = "@@ -1,2 +1,2 @@\n def ping(host):\n- return host\n+ return run(host, shell=True)" + gh = FakeGitHub(files=[{"filename": "app.py", "patch": patch}], + comments=[{"path": "app.py", "line": 2, "original_line": 2, "user": {"login": "cursor[bot]"}, "body": "not passed on"}]) - with tempfile.TemporaryDirectory() as tmp: - out = Path(tmp) / "triage" - semgrep_triage.gather(gh, 7, out) - self.assertEqual(json.loads((out / "commentable-lines.json").read_text()), {"a.go": [[1, 2]]}) - self.assertEqual(json.loads((out / "existing-comments.json").read_text()), - [{"path": "a.go", "line": 2, "original_line": 2, "author": "cursor[bot]"}]) + out = self.root.parent / "triage" + cwd = os.getcwd() + os.chdir(self.root) + try: + semgrep_triage.gather(gh, 7, out, results, self.head) + finally: + os.chdir(cwd) + moved = json.loads(results.read_text()) + self.assertEqual(moved["errors"], []) + self.assertEqual([(f["start"]["line"], f["end"]["line"]) for f in moved["results"]], [(2, 2)]) + head_lines = git(self.root, "show", f"{self.head}:app.py").splitlines() + self.assertEqual(head_lines[1], " return run(host, shell=True)") + self.assertEqual(json.loads((out / "commentable-lines.json").read_text()), {"app.py": [[1, 2]]}) + self.assertEqual(json.loads((out / "existing-comments.json").read_text()), + [{"path": "app.py", "line": 2, "original_line": 2, "author": "cursor[bot]"}]) class ParseResultTest(unittest.TestCase): @@ -181,6 +245,15 @@ def test_checkout_does_not_persist_the_token(self): checkout = self.step("Checkout repository") self.assertIs(checkout["with"]["persist-credentials"], False) + def test_findings_move_to_the_pr_head_that_comments_are_posted_on(self): + job = yaml.safe_load(WORKFLOW.read_text())["jobs"]["triage"] + self.assertEqual(job["env"]["HEAD_SHA"], "${{ github.event.pull_request.head.sha }}") + self.assertEqual(self.step("Checkout repository")["with"]["fetch-depth"], 2) + gather = self.step("Gather PR comments")["run"] + self.assertIn('--head "$HEAD_SHA"', gather) + self.assertLess(gather.index(" gather "), gather.index('git checkout -q --detach "$HEAD_SHA"')) + self.assertIn('--head "$HEAD_SHA"', self.step("Post review")["run"]) + def test_only_runner_steps_get_the_token_and_model_keys_stay_in_the_agent_step(self): for step in self.steps: env = step.get("env", {}) From a658841650abfad352fe46e3cdb82a4698590ec3 Mon Sep 17 00:00:00 2001 From: rgarcia <72655+rgarcia@users.noreply.github.com> Date: Thu, 5 Nov 2026 22:02:15 +0000 Subject: [PATCH 3/3] Read Semgrep paths literally when moving findings onto the PR head --- scripts/semgrep_triage.py | 4 ++-- scripts/test_semgrep_triage.py | 25 +++++++++++++++++-------- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/scripts/semgrep_triage.py b/scripts/semgrep_triage.py index 22973b3..77b26e7 100644 --- a/scripts/semgrep_triage.py +++ b/scripts/semgrep_triage.py @@ -105,8 +105,8 @@ def findings_on_head(results: dict[str, Any], head: str) -> dict[str, Any]: since the PR does not touch them and they cannot be commented on. """ paths = sorted({f["path"] for f in results.get("results", [])}) - diff = subprocess.run(["git", "-c", "core.quotePath=false", "diff", "-U0", "--no-renames", - "--no-ext-diff", "--no-textconv", head, "HEAD", "--", *paths], + diff = subprocess.run(["git", "--literal-pathspecs", "-c", "core.quotePath=false", "diff", "-U0", + "--no-renames", "--no-ext-diff", "--no-textconv", head, "HEAD", "--", *paths], capture_output=True, text=True, check=True).stdout hunks = diff_hunks(diff) kept = [] diff --git a/scripts/test_semgrep_triage.py b/scripts/test_semgrep_triage.py index cc9ca29..00470ca 100644 --- a/scripts/test_semgrep_triage.py +++ b/scripts/test_semgrep_triage.py @@ -77,6 +77,8 @@ def test_old_line(self): class GatherTest(unittest.TestCase): + path = "app.py" + def setUp(self): tmp = tempfile.TemporaryDirectory() self.addCleanup(tmp.cleanup) @@ -101,21 +103,23 @@ def setUp(self): self.assertEqual(git(self.root, "rev-parse", "HEAD^2"), self.head) def write(self, lines): - (self.root / "app.py").write_text("\n".join(lines) + "\n") + file = self.root / self.path + file.parent.mkdir(parents=True, exist_ok=True) + file.write_text("\n".join(lines) + "\n") def finding(self, line): - return {"path": "app.py", "check_id": "shell", + return {"path": self.path, "check_id": "shell", "start": {"line": line, "col": 5}, "end": {"line": line, "col": 9}} def test_moves_findings_onto_the_pr_head_and_writes_pr_context(self): - merge_lines = (self.root / "app.py").read_text().splitlines() + merge_lines = (self.root / self.path).read_text().splitlines() self.assertEqual(merge_lines[4], " return run(host, shell=True)") results = self.root / "semgrep-results.json" # Line 5 is the PR's change; line 9 only exists because the base branch added it. results.write_text(json.dumps({"results": [self.finding(5), self.finding(9)], "errors": []})) patch = "@@ -1,2 +1,2 @@\n def ping(host):\n- return host\n+ return run(host, shell=True)" - gh = FakeGitHub(files=[{"filename": "app.py", "patch": patch}], - comments=[{"path": "app.py", "line": 2, "original_line": 2, + gh = FakeGitHub(files=[{"filename": self.path, "patch": patch}], + comments=[{"path": self.path, "line": 2, "original_line": 2, "user": {"login": "cursor[bot]"}, "body": "not passed on"}]) out = self.root.parent / "triage" cwd = os.getcwd() @@ -127,11 +131,16 @@ def test_moves_findings_onto_the_pr_head_and_writes_pr_context(self): moved = json.loads(results.read_text()) self.assertEqual(moved["errors"], []) self.assertEqual([(f["start"]["line"], f["end"]["line"]) for f in moved["results"]], [(2, 2)]) - head_lines = git(self.root, "show", f"{self.head}:app.py").splitlines() + head_lines = git(self.root, "show", f"{self.head}:{self.path}").splitlines() self.assertEqual(head_lines[1], " return run(host, shell=True)") - self.assertEqual(json.loads((out / "commentable-lines.json").read_text()), {"app.py": [[1, 2]]}) + self.assertEqual(json.loads((out / "commentable-lines.json").read_text()), {self.path: [[1, 2]]}) self.assertEqual(json.loads((out / "existing-comments.json").read_text()), - [{"path": "app.py", "line": 2, "original_line": 2, "author": "cursor[bot]"}]) + [{"path": self.path, "line": 2, "original_line": 2, "author": "cursor[bot]"}]) + + +class PathspecCharacterGatherTest(GatherTest): + # Semgrep paths are file names, not pathspecs: a leading ":" is pathspec magic and "[id]" a glob. + path = ":[id].py" class ParseResultTest(unittest.TestCase):