From f3145933b465e0bac0eebf16eac470395be3d88d Mon Sep 17 00:00:00 2001 From: Ismail Pelaseyed Date: Wed, 20 May 2026 22:46:40 +0200 Subject: [PATCH 1/2] Add finding dismissal flow Support dismissing Superagent findings from trusted review-thread replies and resolved conversations, using P0-P3 labels consistently across comments and checks. --- .flue/agents/pr-scan.ts | 4 +- README.md | 11 +- src/events/index.ts | 10 + src/events/pullRequestReviewComment.ts | 60 +++ src/events/pullRequestReviewThread.ts | 51 ++ src/lib/__tests__/findingPriority.test.ts | 11 + src/lib/db.ts | 39 ++ src/lib/dismissalEval.ts | 110 +++++ src/lib/env.ts | 9 +- src/lib/findingPriority.ts | 12 + src/lib/types.ts | 1 + src/services/__tests__/comments.test.ts | 6 +- .../__tests__/findingDismissal.test.ts | 328 ++++++++++++ src/services/__tests__/prFindings.test.ts | 74 +++ src/services/__tests__/prScan.test.ts | 9 +- .../__tests__/trustedContributor.test.ts | 17 + src/services/comments.ts | 3 +- src/services/findingDismissal.ts | 465 ++++++++++++++++++ src/services/prFindingDismissals.ts | 34 ++ src/services/prFindings.ts | 268 ++++++++++ src/services/prScan.ts | 11 +- src/services/trustedContributor.ts | 47 ++ 22 files changed, 1567 insertions(+), 13 deletions(-) create mode 100644 src/events/pullRequestReviewComment.ts create mode 100644 src/events/pullRequestReviewThread.ts create mode 100644 src/lib/__tests__/findingPriority.test.ts create mode 100644 src/lib/dismissalEval.ts create mode 100644 src/lib/findingPriority.ts create mode 100644 src/services/__tests__/findingDismissal.test.ts create mode 100644 src/services/__tests__/prFindings.test.ts create mode 100644 src/services/__tests__/trustedContributor.test.ts create mode 100644 src/services/findingDismissal.ts create mode 100644 src/services/prFindingDismissals.ts create mode 100644 src/services/prFindings.ts create mode 100644 src/services/trustedContributor.ts diff --git a/.flue/agents/pr-scan.ts b/.flue/agents/pr-scan.ts index 892569b..1d403fe 100644 --- a/.flue/agents/pr-scan.ts +++ b/.flue/agents/pr-scan.ts @@ -171,9 +171,9 @@ Scan exactly these areas: 3. General PR changes that indicate malicious intent. Look for secret exfiltration, obfuscation, encoded payloads, unexpected network calls, dependency confusion, typosquatting, privilege escalation, credential handling changes, telemetry that leaks sensitive data, suspicious binary/blob additions, dangerous eval/exec patterns, and changes that hide behavior from reviewers. Every finding must be actionable: -- title: concise issue name +- title: one clear sentence describing the issue (shown after the priority label, e.g. "**P2:** ...") - category: "ci_cd", "lifecycle", or "malicious_intent" -- severity: "critical", "high", "medium", or "low" +- severity: "critical" (P0), "high" (P1), "medium" (P2), or "low" (P3) - file and line when available - evidence: quote or describe the concrete changed code/pattern - recommendation: exact remediation or review action diff --git a/README.md b/README.md index 8ff6603..4cc64d1 100644 --- a/README.md +++ b/README.md @@ -31,6 +31,7 @@ Superagent then updates the PR with: | ------------------------------------------- | --------------- | -------------------------------------- | ---------------------- | ------------ | | PR has no findings / contributor score >= 30 | success | `pr:verified` / `contributor:verified` | removed | no | | PR has security concerns | action required | `pr:flagged` | inline review comments | yes | +| All findings dismissed in thread replies | success | `pr:verified` | acknowledgment reply | no | | Contributor score < 30 | failure | `contributor:flagged` | posted | yes | | PR scan inconclusive | neutral | -- | -- | no | @@ -48,9 +49,11 @@ Create a new GitHub App at `https://github.com/settings/apps/new` with these set - Contents: Read (for repository config) - Metadata: Read -**Webhook events:** +**Webhook events** (enable every item under the app's **Subscribe to events** - registering handlers in this repo does not subscribe the app): - Pull request +- Pull request review comment +- **Pull request review thread** (required for **Resolve conversation** to dismiss findings) - Check run - Check suite - Installation @@ -140,6 +143,12 @@ src/ └── policy.ts # Check-result evaluation logic ``` +## Dismissing findings + +When Superagent flags a finding on an inline review comment, reply in that thread with context (for example, that the behavior is intentional). Superagent evaluates the reply and, when it adequately explains the concern, posts a short acknowledgment and clears the **Security scan** check once every open finding on the PR has been addressed. + +Re-scanning the PR (a new push or re-run) replaces findings and clears prior dismissals for that pull request. + ## Re-running checks Maintainers can re-run any Superagent check from the GitHub UI by clicking "Re-run" on the check run. The app handles `check_run.rerequested` events and re-executes the corresponding scan. diff --git a/src/events/index.ts b/src/events/index.ts index ec4db45..d97150e 100644 --- a/src/events/index.ts +++ b/src/events/index.ts @@ -2,12 +2,22 @@ import type { App } from "octokit"; import { handlePullRequest } from "./pullRequest.js"; import { handleCheckRunRerequested } from "./checkRun.js"; import { handleInstallationCreated, handleInstallationDeleted } from "./installation.js"; +import { handlePullRequestReviewComment } from "./pullRequestReviewComment.js"; +import { handlePullRequestReviewThread } from "./pullRequestReviewThread.js"; import { logger } from "../lib/logger.js"; type WebhookHandler = (event: any) => Promise; export function registerEventHandlers(app: App) { app.webhooks.on("pull_request", handlePullRequest as WebhookHandler); + app.webhooks.on( + "pull_request_review_comment", + handlePullRequestReviewComment as WebhookHandler, + ); + app.webhooks.on( + "pull_request_review_thread", + handlePullRequestReviewThread as WebhookHandler, + ); app.webhooks.on("check_run.rerequested", handleCheckRunRerequested as WebhookHandler); app.webhooks.on("installation.created", handleInstallationCreated as WebhookHandler); app.webhooks.on("installation.deleted", handleInstallationDeleted as WebhookHandler); diff --git a/src/events/pullRequestReviewComment.ts b/src/events/pullRequestReviewComment.ts new file mode 100644 index 0000000..e4be3c9 --- /dev/null +++ b/src/events/pullRequestReviewComment.ts @@ -0,0 +1,60 @@ +import type { Octokit } from "octokit"; +import { + handleFindingReply, + reconcilePrScanAfterDismissals, +} from "../services/findingDismissal.js"; +import { childLogger } from "../lib/logger.js"; + +export async function handlePullRequestReviewComment({ + octokit, + payload, +}: { + octokit: Octokit; + payload: Record; +}) { + if (payload.action !== "created") return; + + const comment = payload.comment as Record | undefined; + const pullRequest = payload.pull_request as Record | undefined; + const repository = payload.repository as Record | undefined; + if (!comment || !pullRequest || !repository) return; + + const owner = (repository.owner as { login?: string })?.login; + const repo = repository.name as string | undefined; + const prNumber = pullRequest.number as number | undefined; + if (!owner || !repo || prNumber == null) return; + + const log = childLogger({ + event: "pull_request_review_comment", + owner, + repo, + pr: prNumber, + commentId: comment.id, + }); + + try { + await handleFindingReply(octokit, { + owner, + repo, + prNumber, + comment: { + id: comment.id as number, + body: (comment.body as string | null) ?? null, + in_reply_to_id: (comment.in_reply_to_id as number | null) ?? null, + path: comment.path as string | undefined, + line: (comment.line as number | null) ?? null, + user: comment.user as { login?: string; type?: string } | null, + author_association: (comment.author_association as string | null) ?? null, + }, + }); + } catch (err) { + log.error({ err }, "Failed to handle finding reply"); + throw err; + } + + try { + await reconcilePrScanAfterDismissals(octokit, { owner, repo, prNumber }); + } catch (err) { + log.warn({ err }, "Failed to reconcile PR scan after review comment"); + } +} diff --git a/src/events/pullRequestReviewThread.ts b/src/events/pullRequestReviewThread.ts new file mode 100644 index 0000000..66cc1d4 --- /dev/null +++ b/src/events/pullRequestReviewThread.ts @@ -0,0 +1,51 @@ +import type { Octokit } from "octokit"; +import { childLogger } from "../lib/logger.js"; +import { handleFindingThreadResolved } from "../services/findingDismissal.js"; + +export async function handlePullRequestReviewThread({ + octokit, + payload, +}: { + octokit: Octokit; + payload: Record; +}) { + if (payload.action !== "resolved") return; + + const thread = payload.thread as Record | undefined; + const pullRequest = payload.pull_request as Record | undefined; + const repository = payload.repository as Record | undefined; + const sender = payload.sender as { login?: string; type?: string } | null; + if (!thread || !pullRequest || !repository) return; + + const owner = (repository.owner as { login?: string })?.login; + const repo = repository.name as string | undefined; + const prNumber = pullRequest.number as number | undefined; + if (!owner || !repo || prNumber == null) return; + + const log = childLogger({ + event: "pull_request_review_thread", + action: payload.action, + owner, + repo, + pr: prNumber, + threadId: thread.node_id ?? thread.id, + }); + + try { + await handleFindingThreadResolved(octokit, { + owner, + repo, + prNumber, + thread: { + id: thread.id as string | number | null, + node_id: (thread.node_id as string | null) ?? null, + path: (thread.path as string | null) ?? null, + line: (thread.line as number | null) ?? null, + }, + sender, + }); + } catch (err) { + log.error({ err }, "Failed to handle resolved review thread"); + throw err; + } +} diff --git a/src/lib/__tests__/findingPriority.test.ts b/src/lib/__tests__/findingPriority.test.ts new file mode 100644 index 0000000..321672b --- /dev/null +++ b/src/lib/__tests__/findingPriority.test.ts @@ -0,0 +1,11 @@ +import { describe, expect, it } from "vitest"; +import { formatFindingPriority } from "../findingPriority.js"; + +describe("formatFindingPriority", () => { + it("maps severities to P0-P3", () => { + expect(formatFindingPriority("critical")).toBe("P0"); + expect(formatFindingPriority("high")).toBe("P1"); + expect(formatFindingPriority("medium")).toBe("P2"); + expect(formatFindingPriority("low")).toBe("P3"); + }); +}); diff --git a/src/lib/db.ts b/src/lib/db.ts index 2dcde2a..486a018 100644 --- a/src/lib/db.ts +++ b/src/lib/db.ts @@ -40,6 +40,17 @@ db.exec(` result_json TEXT NOT NULL, scanned_at TEXT NOT NULL ); + + CREATE TABLE IF NOT EXISTS pr_finding_dismissals ( + owner TEXT NOT NULL, + repo TEXT NOT NULL, + pr_number INTEGER NOT NULL, + review_comment_id INTEGER NOT NULL, + dismissed_by TEXT NOT NULL, + head_sha TEXT NOT NULL, + dismissed_at TEXT NOT NULL DEFAULT (datetime('now')), + PRIMARY KEY (owner, repo, pr_number, review_comment_id) + ); `); logger.info({ path: env.dbPath }, "Database initialized"); @@ -118,6 +129,34 @@ export const queries: Record = { result_json = @resultJson, scanned_at = @scannedAt `), + + dismissPrFinding: db.prepare(` + INSERT INTO pr_finding_dismissals ( + owner, repo, pr_number, review_comment_id, dismissed_by, head_sha + ) + VALUES ( + @owner, @repo, @prNumber, @reviewCommentId, @dismissedBy, @headSha + ) + ON CONFLICT(owner, repo, pr_number, review_comment_id) DO UPDATE SET + dismissed_by = @dismissedBy, + head_sha = @headSha, + dismissed_at = datetime('now') + `), + + isPrFindingDismissed: db.prepare(` + SELECT 1 + FROM pr_finding_dismissals + WHERE owner = @owner + AND repo = @repo + AND pr_number = @prNumber + AND review_comment_id = @reviewCommentId + LIMIT 1 + `), + + clearPrFindingDismissals: db.prepare(` + DELETE FROM pr_finding_dismissals + WHERE owner = @owner AND repo = @repo AND pr_number = @prNumber + `), }; export function saveInstallation( diff --git a/src/lib/dismissalEval.ts b/src/lib/dismissalEval.ts new file mode 100644 index 0000000..0417980 --- /dev/null +++ b/src/lib/dismissalEval.ts @@ -0,0 +1,110 @@ +import { getAzureKimiProviderConfig } from "./azureKimi.js"; +import { childLogger } from "./logger.js"; + +const log = childLogger({ service: "dismissal-eval" }); + +export interface DismissalEvaluation { + dismiss: boolean; + acknowledgment: string; +} + +export interface DismissalEvalOptions { + trustedContributor?: boolean; +} + +function buildSystemPrompt(trustedContributor: boolean): string { + const base = + "You review developer replies on pull request security findings. " + + 'Return JSON only: {"dismiss":boolean,"acknowledgment":string}. ' + + "acknowledgment must be one short, natural sentence addressed to the developer. " + + "Vary wording; do not repeat the same stock phrase every time."; + + if (trustedContributor) { + return ( + `${base} ` + + "The replier is a trusted repository contributor (owner, member, collaborator, or PR author). " + + "Be permissive: set dismiss true when they give any good-faith explanation, including brief notes like intentional or acceptable risk. " + + "Only set dismiss false if the reply is empty, off-topic, or clearly not addressing the finding. " + + "When dismiss is true, write a concise acknowledgment that reflects what they said (e.g. noted it's intentional, test fixture, or accepted risk)." + ); + } + + return ( + `${base} ` + + "Be reasonably permissive for good-faith replies, but require enough context to understand why the finding should not block merge. " + + "Set dismiss true when the reply explains intentional design, acceptable risk, or a false positive. " + + "When dismiss is false, acknowledgment should briefly ask for the missing context." + ); +} + +export async function evaluateFindingDismissal( + findingBody: string, + replyBody: string, + options: DismissalEvalOptions = {}, +): Promise { + let config; + try { + config = getAzureKimiProviderConfig(); + } catch (err) { + log.warn({ err }, "Azure OpenAI not configured; skipping dismissal evaluation"); + return null; + } + + const trustedContributor = options.trustedContributor ?? false; + + const response = await fetch(`${config.baseUrl}/chat/completions`, { + method: "POST", + headers: { + "Content-Type": "application/json", + Authorization: `Bearer ${config.apiKey}`, + }, + body: JSON.stringify({ + model: config.modelId, + response_format: { type: "json_object" }, + messages: [ + { + role: "system", + content: buildSystemPrompt(trustedContributor), + }, + { + role: "user", + content: `Security finding:\n${findingBody}\n\nDeveloper reply:\n${replyBody}`, + }, + ], + }), + }); + + if (!response.ok) { + log.warn({ status: response.status }, "Dismissal evaluation request failed"); + return null; + } + + const payload = (await response.json()) as { + choices?: Array<{ message?: { content?: string } }>; + }; + const content = payload.choices?.[0]?.message?.content; + if (!content) return null; + + try { + const parsed = JSON.parse(content) as { dismiss?: boolean; acknowledgment?: string }; + if (typeof parsed.dismiss !== "boolean") return null; + + let dismiss = parsed.dismiss; + if (trustedContributor && !dismiss) { + dismiss = true; + log.info("Overriding dismiss to true for trusted contributor"); + } + + const acknowledgment = + typeof parsed.acknowledgment === "string" && parsed.acknowledgment.trim() + ? parsed.acknowledgment.trim() + : dismiss + ? "Got it, thanks for the context." + : "Thanks for the reply. I still need a bit more context on why this finding is acceptable before I can clear it."; + + return { dismiss, acknowledgment }; + } catch (err) { + log.warn({ err, content }, "Failed to parse dismissal evaluation"); + return null; + } +} diff --git a/src/lib/env.ts b/src/lib/env.ts index f97e79c..2d219a6 100644 --- a/src/lib/env.ts +++ b/src/lib/env.ts @@ -1,3 +1,6 @@ +import os from "node:os"; +import path from "node:path"; + function required(key: string): string { const value = process.env[key]; if (!value) throw new Error(`Missing required environment variable: ${key}`); @@ -27,6 +30,10 @@ export const env = { return process.env.LOG_LEVEL ?? "info"; }, get dbPath() { - return process.env.DB_PATH ?? "/data/brin.db"; + if (process.env.DB_PATH) return process.env.DB_PATH; + if (process.env.NODE_ENV === "test") { + return path.join(os.tmpdir(), "brin-github-test.db"); + } + return "/data/brin.db"; }, } as const; diff --git a/src/lib/findingPriority.ts b/src/lib/findingPriority.ts new file mode 100644 index 0000000..69b823a --- /dev/null +++ b/src/lib/findingPriority.ts @@ -0,0 +1,12 @@ +import type { PrFindingSeverity } from "./types.js"; + +const SEVERITY_TO_PRIORITY: Record = { + critical: "P0", + high: "P1", + medium: "P2", + low: "P3", +}; + +export function formatFindingPriority(severity: PrFindingSeverity): string { + return SEVERITY_TO_PRIORITY[severity]; +} diff --git a/src/lib/types.ts b/src/lib/types.ts index 3ee8bfc..b343c7e 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -6,6 +6,7 @@ export const CHECK_NAMES = { export const MARKERS = { PR_SCAN: "", PR_FINDING: "", + PR_FINDING_ACK: "", CONTRIBUTOR_TRUST: "", } as const; diff --git a/src/services/__tests__/comments.test.ts b/src/services/__tests__/comments.test.ts index 29660ef..fadebe0 100644 --- a/src/services/__tests__/comments.test.ts +++ b/src/services/__tests__/comments.test.ts @@ -33,11 +33,11 @@ describe("renderPrScanComment", () => { expect(body).toContain("suspicious changes"); expect(body).not.toContain("Score"); expect(body).not.toContain("Verdict"); - expect(body).toContain("[CRITICAL] pull_request_target checks out fork code"); + expect(body).toContain("**P0:** pull_request_target checks out fork code"); expect(body).toContain("**Category:** CI/CD"); expect(body).toContain("`.github/workflows/ci.yml:12`"); expect(body).toContain("Use pull_request for untrusted code"); - expect(body).toContain("[HIGH] Encoded payload added to build script"); + expect(body).toContain("**P1:** Encoded payload added to build script"); expect(body).toContain("Analyzed by [Superagent]"); }); @@ -45,7 +45,7 @@ describe("renderPrScanComment", () => { const result: PrScanResult = { findings: [] }; const body = renderPrScanComment("review", result); - expect(body).not.toContain("#### ["); + expect(body).not.toContain("#### "); }); }); diff --git a/src/services/__tests__/findingDismissal.test.ts b/src/services/__tests__/findingDismissal.test.ts new file mode 100644 index 0000000..6cdafbf --- /dev/null +++ b/src/services/__tests__/findingDismissal.test.ts @@ -0,0 +1,328 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { CHECK_NAMES, LABEL_DEFS, MARKERS } from "../../lib/types.js"; + +const dismissedFindingIds = vi.hoisted(() => new Set()); + +vi.mock("../prFindingDismissals.js", () => ({ + dismissPrFinding: ({ reviewCommentId }: { reviewCommentId: number }) => { + dismissedFindingIds.add(reviewCommentId); + }, + isPrFindingDismissed: ( + _owner: string, + _repo: string, + _prNumber: number, + reviewCommentId: number, + ) => dismissedFindingIds.has(reviewCommentId), + clearPrFindingDismissals: () => { + dismissedFindingIds.clear(); + }, +})); + +vi.mock("../../lib/logger.js", () => ({ + childLogger: () => ({ + error: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + }), +})); + +vi.mock("../../lib/dismissalEval.js", () => ({ + evaluateFindingDismissal: vi.fn(), +})); + +vi.mock("../config.js", () => ({ + loadConfig: vi.fn().mockResolvedValue({ + prScan: { enabled: true }, + contributorTrust: { enabled: true, blockBelowScore: 30, trustedAuthors: [] }, + comments: { mode: "detailed" }, + }), +})); + +vi.mock("../trustedContributor.js", () => ({ + isTrustedRepoContributor: vi.fn().mockResolvedValue(false), +})); + +import { evaluateFindingDismissal } from "../../lib/dismissalEval.js"; +import { isTrustedRepoContributor } from "../trustedContributor.js"; +import { + handleFindingThreadResolved, + handleFindingReply, + reconcilePrScanAfterDismissals, +} from "../findingDismissal.js"; + +describe("handleFindingReply", () => { + beforeEach(() => { + vi.clearAllMocks(); + dismissedFindingIds.clear(); + }); + + it("dismisses a finding, replies, and clears the check when all findings are addressed", async () => { + vi.mocked(evaluateFindingDismissal).mockResolvedValue({ + dismiss: true, + acknowledgment: "Thanks for clarifying, understood it's metadata only for now.", + }); + + const octokit = mockOctokit({ + findingComments: [ + { + id: 10, + body: `${MARKERS.PR_FINDING}\n**P2:** Sandbox scopes`, + path: "pkg.ts", + line: 1, + }, + ], + }); + + await handleFindingReply(octokit, { + owner: "acme", + repo: "repo", + prNumber: 12, + comment: { + id: 11, + body: "We're just using it for metadata for now.", + in_reply_to_id: 10, + user: { login: "marcindobry", type: "User" }, + }, + }); + + expect(evaluateFindingDismissal).toHaveBeenCalled(); + expect(octokit.rest.reactions.createForPullRequestReviewComment).toHaveBeenCalledWith( + expect.objectContaining({ + comment_id: 11, + content: "eyes", + }), + ); + expect(octokit.rest.pulls.createReviewComment).toHaveBeenCalledWith( + expect.objectContaining({ + in_reply_to: 10, + body: expect.stringContaining("metadata only for now"), + }), + ); + expect(octokit.rest.checks.update).toHaveBeenCalledWith( + expect.objectContaining({ + conclusion: "success", + output: expect.objectContaining({ + summary: "All reported findings were reviewed and dismissed.", + }), + }), + ); + expect(octokit.rest.issues.setLabels).toHaveBeenCalledWith( + expect.objectContaining({ + labels: expect.arrayContaining([LABEL_DEFS.PR_VERIFIED.name]), + }), + ); + }); + + it("uses permissive model evaluation for trusted contributor replies", async () => { + vi.mocked(isTrustedRepoContributor).mockResolvedValueOnce(true); + vi.mocked(evaluateFindingDismissal).mockResolvedValue({ + dismiss: true, + acknowledgment: "Makes sense, treating this as an intentional test fixture.", + }); + + const octokit = mockOctokit(); + await handleFindingReply(octokit, { + owner: "acme", + repo: "repo", + prNumber: 12, + comment: { + id: 11, + body: "This is intentional", + in_reply_to_id: 10, + author_association: "CONTRIBUTOR", + user: { login: "homanp", type: "User" }, + }, + }); + + expect(evaluateFindingDismissal).toHaveBeenCalledWith( + expect.any(String), + "This is intentional", + { trustedContributor: true }, + ); + expect(octokit.rest.pulls.createReviewComment).toHaveBeenCalledWith( + expect.objectContaining({ + body: expect.stringContaining("intentional test fixture"), + }), + ); + }); + + it("replies when dismissal is not accepted", async () => { + vi.mocked(evaluateFindingDismissal).mockResolvedValue({ + dismiss: false, + acknowledgment: "Please explain why the PR title interpolation is safe here.", + }); + + const octokit = mockOctokit(); + await handleFindingReply(octokit, { + owner: "acme", + repo: "repo", + prNumber: 12, + comment: { + id: 11, + body: "intentional", + in_reply_to_id: 10, + user: { login: "stranger", type: "User" }, + }, + }); + + expect(octokit.rest.pulls.createReviewComment).toHaveBeenCalledWith( + expect.objectContaining({ + body: expect.stringContaining("PR title interpolation"), + }), + ); + expect(octokit.rest.checks.update).not.toHaveBeenCalled(); + }); + + it("ignores bot replies", async () => { + const octokit = mockOctokit(); + await handleFindingReply(octokit, { + owner: "acme", + repo: "repo", + prNumber: 12, + comment: { + id: 11, + body: "Thanks", + in_reply_to_id: 10, + user: { login: "superagent-security[bot]", type: "Bot" }, + }, + }); + expect(evaluateFindingDismissal).not.toHaveBeenCalled(); + }); +}); + +describe("handleFindingThreadResolved", () => { + beforeEach(() => { + vi.clearAllMocks(); + dismissedFindingIds.clear(); + }); + + it("dismisses a finding when a trusted contributor resolves the thread", async () => { + vi.mocked(isTrustedRepoContributor).mockResolvedValueOnce(true); + const octokit = mockOctokit({ + findingComments: [ + { id: 10, body: `${MARKERS.PR_FINDING}\n**P2:** one`, path: "a.ts", line: 1 }, + ], + }); + + await handleFindingThreadResolved(octokit, { + owner: "acme", + repo: "repo", + prNumber: 12, + thread: { path: "a.ts", line: 1 }, + sender: { login: "maintainer", type: "User" }, + }); + + expect(octokit.rest.checks.update).toHaveBeenCalledWith( + expect.objectContaining({ + conclusion: "success", + output: expect.objectContaining({ + summary: "All reported findings were reviewed and dismissed.", + }), + }), + ); + expect(evaluateFindingDismissal).not.toHaveBeenCalled(); + }); + + it("ignores resolved threads from untrusted users", async () => { + vi.mocked(isTrustedRepoContributor).mockResolvedValueOnce(false); + const octokit = mockOctokit(); + + await handleFindingThreadResolved(octokit, { + owner: "acme", + repo: "repo", + prNumber: 12, + thread: { path: "pkg.ts", line: 1 }, + sender: { login: "drive-by", type: "User" }, + }); + + expect(octokit.rest.checks.update).not.toHaveBeenCalled(); + }); +}); + +describe("reconcilePrScanAfterDismissals", () => { + beforeEach(() => { + dismissedFindingIds.clear(); + }); + + it("does not clear the check while open findings remain", async () => { + dismissedFindingIds.add(10); + + const octokit = mockOctokit({ + findingComments: [ + { id: 10, body: `${MARKERS.PR_FINDING}\n**P2:** one`, path: "a.ts", line: 1 }, + { id: 20, body: `${MARKERS.PR_FINDING}\n**P1:** two`, path: "b.ts", line: 2 }, + ], + }); + + await reconcilePrScanAfterDismissals(octokit, { + owner: "acme", + repo: "repo", + prNumber: 12, + headSha: "abc123", + }); + + expect(octokit.rest.checks.update).not.toHaveBeenCalled(); + }); +}); + +function mockOctokit(options?: { + findingComments?: Array<{ + id: number; + body: string; + path: string; + line: number; + in_reply_to_id?: number; + }>; +}) { + const findingComments = options?.findingComments ?? [ + { + id: 10, + body: `${MARKERS.PR_FINDING}\n**P2:** Sandbox scopes`, + path: "pkg.ts", + line: 1, + }, + ]; + const commentById = new Map( + findingComments.map((comment) => [comment.id, comment]), + ); + const listReviewComments = vi.fn(); + + return { + paginate: vi.fn((endpoint) => { + if (endpoint === listReviewComments) { + return Promise.resolve(findingComments); + } + return Promise.resolve([]); + }), + rest: { + pulls: { + listReviewComments, + getReviewComment: vi.fn().mockImplementation(({ comment_id }) => { + const comment = commentById.get(comment_id); + if (!comment) throw new Error(`missing comment ${comment_id}`); + return Promise.resolve({ data: comment }); + }), + get: vi.fn().mockResolvedValue({ data: { head: { sha: "abc123" } } }), + createReviewComment: vi.fn().mockResolvedValue({}), + createReplyForReviewComment: vi.fn().mockResolvedValue({}), + }, + checks: { + listForRef: vi.fn().mockResolvedValue({ + data: { + check_runs: [{ id: 99, name: CHECK_NAMES.PR_SCAN }], + }, + }), + update: vi.fn().mockResolvedValue({}), + }, + issues: { + setLabels: vi.fn().mockResolvedValue({}), + getLabel: vi.fn().mockResolvedValue({ data: { name: LABEL_DEFS.PR_VERIFIED.name } }), + createLabel: vi.fn().mockResolvedValue({}), + listLabelsOnIssue: vi.fn().mockResolvedValue({ data: [] }), + }, + reactions: { + createForPullRequestReviewComment: vi.fn().mockResolvedValue({}), + }, + }, + } as any; +} diff --git a/src/services/__tests__/prFindings.test.ts b/src/services/__tests__/prFindings.test.ts new file mode 100644 index 0000000..7403b46 --- /dev/null +++ b/src/services/__tests__/prFindings.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, it, vi } from "vitest"; +import { MARKERS } from "../../lib/types.js"; +import { + isPrFindingComment, + resolveFindingForUserReply, + resolveRootFindingComment, +} from "../prFindings.js"; + +describe("isPrFindingComment", () => { + it("matches finding comments but not acknowledgments", () => { + expect(isPrFindingComment(`${MARKERS.PR_FINDING}\n**P2:** issue`)).toBe(true); + expect(isPrFindingComment(`${MARKERS.PR_FINDING_ACK}\nThanks`)).toBe(false); + }); +}); + +describe("resolveRootFindingComment", () => { + it("walks reply chains to the root finding", async () => { + const octokit = { + rest: { + pulls: { + getReviewComment: vi + .fn() + .mockResolvedValueOnce({ + data: { + id: 11, + body: "reply", + in_reply_to_id: 10, + }, + }) + .mockResolvedValueOnce({ + data: { + id: 10, + body: `${MARKERS.PR_FINDING}\n**P2:** root`, + in_reply_to_id: null, + }, + }), + }, + }, + } as any; + + const root = await resolveRootFindingComment(octokit, "acme", "repo", { + id: 11, + body: "reply", + in_reply_to_id: 10, + }); + + expect(root?.id).toBe(10); + }); +}); + +describe("resolveFindingForUserReply", () => { + it("matches a finding on the same file line when the reply has no in_reply_to_id", async () => { + const octokit = { + paginate: vi.fn().mockResolvedValue([ + { + id: 10, + body: `${MARKERS.PR_FINDING}\n**P0:** issue`, + path: ".github/workflows/test.yml", + line: 17, + }, + ]), + rest: { pulls: { listReviewComments: vi.fn() } }, + } as any; + + const finding = await resolveFindingForUserReply(octokit, "acme", "repo", 12, { + id: 11, + body: "This is intentional", + path: ".github/workflows/test.yml", + line: 17, + }); + + expect(finding?.id).toBe(10); + }); +}); diff --git a/src/services/__tests__/prScan.test.ts b/src/services/__tests__/prScan.test.ts index 325dc48..8bfa0e9 100644 --- a/src/services/__tests__/prScan.test.ts +++ b/src/services/__tests__/prScan.test.ts @@ -2,6 +2,10 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { CHECK_NAMES, DEFAULT_CONFIG } from "../../lib/types.js"; import { runPrScan } from "../prScan.js"; +vi.mock("../prFindingDismissals.js", () => ({ + clearPrFindingDismissals: vi.fn(), +})); + vi.mock("../../lib/logger.js", () => ({ childLogger: () => ({ error: vi.fn(), @@ -82,8 +86,9 @@ describe("runPrScan", () => { ); const body = octokit.rest.pulls.createReview.mock.calls[0][0].comments[0].body; expect(body).toContain("A new postinstall hook executes a remote script."); - expect(body).toContain("Fix: Remove the lifecycle hook"); - expect(body).toContain("[HIGH] Suspicious lifecycle hook"); + expect(body).toContain("Remove the lifecycle hook"); + expect(body).toContain("**P1:** Suspicious lifecycle hook"); + expect(body).not.toContain("Fix:"); expect(body).not.toContain("Recommended fix"); }); diff --git a/src/services/__tests__/trustedContributor.test.ts b/src/services/__tests__/trustedContributor.test.ts new file mode 100644 index 0000000..4366e47 --- /dev/null +++ b/src/services/__tests__/trustedContributor.test.ts @@ -0,0 +1,17 @@ +import { describe, expect, it } from "vitest"; +import { isTrustedAssociation } from "../trustedContributor.js"; + +describe("isTrustedAssociation", () => { + it("trusts repository contributors and maintainers", () => { + expect(isTrustedAssociation("OWNER")).toBe(true); + expect(isTrustedAssociation("MEMBER")).toBe(true); + expect(isTrustedAssociation("COLLABORATOR")).toBe(true); + expect(isTrustedAssociation("CONTRIBUTOR")).toBe(true); + }); + + it("does not trust drive-by or first-time authors", () => { + expect(isTrustedAssociation("NONE")).toBe(false); + expect(isTrustedAssociation("FIRST_TIMER")).toBe(false); + expect(isTrustedAssociation(undefined)).toBe(false); + }); +}); diff --git a/src/services/comments.ts b/src/services/comments.ts index b84d150..f6e8367 100644 --- a/src/services/comments.ts +++ b/src/services/comments.ts @@ -1,5 +1,6 @@ import type { Octokit } from "octokit"; import type { PrScanResult, ContributorResult, PrStatus } from "../lib/types.js"; +import { formatFindingPriority } from "../lib/findingPriority.js"; import { MARKERS } from "../lib/types.js"; export async function upsertComment( @@ -68,7 +69,7 @@ export function renderPrScanComment( body += `This PR has suspicious changes that should be reviewed before merge.\n\n`; for (const finding of findings) { - body += `#### [${finding.severity.toUpperCase()}] ${finding.title}\n\n`; + body += `**${formatFindingPriority(finding.severity)}:** ${finding.title}\n\n`; body += `- **Category:** ${formatFindingCategory(finding.category)}\n`; if (finding.file) { body += `- **Location:** \`${finding.file}`; diff --git a/src/services/findingDismissal.ts b/src/services/findingDismissal.ts new file mode 100644 index 0000000..7103c87 --- /dev/null +++ b/src/services/findingDismissal.ts @@ -0,0 +1,465 @@ +import type { Octokit } from "octokit"; +import { evaluateFindingDismissal } from "../lib/dismissalEval.js"; +import { childLogger } from "../lib/logger.js"; +import { CHECK_NAMES, LABEL_DEFS, MARKERS } from "../lib/types.js"; +import { completeCheck } from "./checkRuns.js"; +import { loadConfig } from "./config.js"; +import { + getReviewThreadIdForComment, + listPrFindingComments, + listResolvedFindingCommentIds, + resolveFindingForUserReply, + resolveFindingForReviewThread, + type PrReviewComment, +} from "./prFindings.js"; +import { + dismissPrFinding, + isPrFindingDismissed, +} from "./prFindingDismissals.js"; +import { ensureLabels, setLabel } from "./labels.js"; +import { isTrustedRepoContributor } from "./trustedContributor.js"; + +const PR_LABELS = [LABEL_DEFS.PR_VERIFIED, LABEL_DEFS.PR_FLAGGED]; +const PR_LABEL_NAMES = PR_LABELS.map((l) => l.name); + +const NEED_MORE_DETAIL_REPLY = + "Thanks for the reply. I still need a bit more context on why this finding is acceptable before I can clear it. Could you expand on the intent or risk?"; + +function isBotUser(user: PrReviewComment["user"]): boolean { + return user?.type === "Bot" || !!user?.login?.endsWith("[bot]"); +} + +export async function handleFindingReply( + octokit: Octokit, + params: { + owner: string; + repo: string; + prNumber: number; + comment: PrReviewComment; + }, +): Promise { + const { owner, repo, prNumber, comment } = params; + const log = childLogger({ + service: "finding-dismissal", + owner, + repo, + pr: prNumber, + commentId: comment.id, + }); + + if (isBotUser(comment.user)) return; + + const config = await loadConfig(octokit, owner, repo); + if (!config.prScan.enabled) return; + + const rootFinding = await resolveFindingForUserReply( + octokit, + owner, + repo, + prNumber, + comment, + ); + if (!rootFinding) { + log.info("Reply is not on a Superagent finding thread"); + return; + } + if (isPrFindingDismissed(owner, repo, prNumber, rootFinding.id)) { + log.info({ findingCommentId: rootFinding.id }, "Finding already dismissed"); + await reconcilePrScanAfterDismissals(octokit, { owner, repo, prNumber }); + return; + } + + await addEyesReaction(octokit, owner, repo, comment.id, log); + + const login = comment.user?.login ?? ""; + const trusted = login + ? await isTrustedRepoContributor(octokit, { + owner, + repo, + prNumber, + login, + authorAssociation: comment.author_association, + }) + : false; + + const evaluation = await evaluateFindingDismissal( + rootFinding.body ?? "", + comment.body ?? "", + { trustedContributor: trusted }, + ); + + if (trusted && !evaluation) { + log.info({ login }, "Trusted contributor reply; dismissing with fallback acknowledgment"); + await dismissFinding(octokit, { + owner, + repo, + prNumber, + rootFindingId: rootFinding.id, + replyToCommentId: comment.id, + dismissedBy: login || "unknown", + acknowledgment: "Got it, thanks for the context.", + }); + await reconcilePrScanAfterDismissals(octokit, { owner, repo, prNumber }); + return; + } + + if (evaluation?.dismiss) { + await dismissFinding(octokit, { + owner, + repo, + prNumber, + rootFindingId: rootFinding.id, + replyToCommentId: comment.id, + dismissedBy: login || "unknown", + acknowledgment: evaluation.acknowledgment, + }); + log.info({ findingCommentId: rootFinding.id }, "Dismissed finding after reply"); + await reconcilePrScanAfterDismissals(octokit, { owner, repo, prNumber }); + return; + } + + const followUp = + evaluation?.acknowledgment?.trim() || NEED_MORE_DETAIL_REPLY; + await postFindingReply(octokit, { + owner, + repo, + prNumber, + rootFindingId: rootFinding.id, + replyToCommentId: comment.id, + body: followUp, + }); + log.info("Replied on finding thread without dismissing"); +} + +export async function handleFindingThreadResolved( + octokit: Octokit, + params: { + owner: string; + repo: string; + prNumber: number; + thread: { + id?: string | number | null; + node_id?: string | null; + path?: string | null; + line?: number | null; + }; + sender: { login?: string; type?: string } | null; + }, +): Promise { + const { owner, repo, prNumber, thread, sender } = params; + const log = childLogger({ + service: "finding-dismissal", + owner, + repo, + pr: prNumber, + threadId: thread.node_id ?? thread.id, + }); + + if (isBotUser(sender)) return; + + const config = await loadConfig(octokit, owner, repo); + if (!config.prScan.enabled) return; + + const login = sender?.login ?? ""; + const trusted = login + ? await isTrustedRepoContributor(octokit, { + owner, + repo, + prNumber, + login, + }) + : false; + if (!trusted) { + log.info({ login }, "Ignoring resolved thread from untrusted user"); + return; + } + + const rootFinding = await resolveFindingForReviewThread( + octokit, + owner, + repo, + prNumber, + thread, + ); + if (!rootFinding) { + log.info("Resolved thread is not a Superagent finding"); + return; + } + + if (isPrFindingDismissed(owner, repo, prNumber, rootFinding.id)) { + await reconcilePrScanAfterDismissals(octokit, { owner, repo, prNumber }); + return; + } + + const { data: pr } = await octokit.rest.pulls.get({ + owner, + repo, + pull_number: prNumber, + }); + + dismissPrFinding({ + owner, + repo, + prNumber, + reviewCommentId: rootFinding.id, + dismissedBy: login || "unknown", + headSha: pr.head.sha, + }); + + log.info( + { login, findingCommentId: rootFinding.id }, + "Dismissed finding from resolved thread", + ); + await reconcilePrScanAfterDismissals(octokit, { + owner, + repo, + prNumber, + headSha: pr.head.sha, + }); +} + +async function dismissFinding( + octokit: Octokit, + params: { + owner: string; + repo: string; + prNumber: number; + rootFindingId: number; + replyToCommentId: number; + dismissedBy: string; + acknowledgment: string; + }, +): Promise { + const { owner, repo, prNumber, rootFindingId, replyToCommentId, dismissedBy, acknowledgment } = + params; + + const { data: pr } = await octokit.rest.pulls.get({ + owner, + repo, + pull_number: prNumber, + }); + + dismissPrFinding({ + owner, + repo, + prNumber, + reviewCommentId: rootFindingId, + dismissedBy, + headSha: pr.head.sha, + }); + + await postFindingReply(octokit, { + owner, + repo, + prNumber, + rootFindingId, + replyToCommentId, + body: acknowledgment, + }); +} + +async function postFindingReply( + octokit: Octokit, + params: { + owner: string; + repo: string; + prNumber: number; + rootFindingId: number; + replyToCommentId: number; + body: string; + }, +): Promise { + const body = `${params.body}\n\n${MARKERS.PR_FINDING_ACK}`; + + try { + await octokit.rest.pulls.createReviewComment({ + owner: params.owner, + repo: params.repo, + pull_number: params.prNumber, + body, + in_reply_to: params.rootFindingId, + } as Parameters[0]); + return; + } catch { + // Fall back to the dedicated REST reply endpoint below. + } + + try { + await octokit.rest.pulls.createReplyForReviewComment({ + owner: params.owner, + repo: params.repo, + pull_number: params.prNumber, + comment_id: params.rootFindingId, + body, + }); + return; + } catch { + // Fall back to GraphQL review-thread replies below. + } + + const threadId = await getReviewThreadIdForComment( + octokit, + params.owner, + params.repo, + params.prNumber, + params.rootFindingId, + ); + + if (!threadId) throw new Error("Unable to resolve review thread for finding reply"); + + await octokit.graphql( + `mutation($threadId: ID!, $body: String!) { + addPullRequestReviewThreadReply(input: { + pullRequestReviewThreadId: $threadId, + body: $body + }) { + comment { + id + } + } + }`, + { + threadId, + body, + }, + ); +} + +export async function reconcilePrScanAfterDismissals( + octokit: Octokit, + params: { + owner: string; + repo: string; + prNumber: number; + headSha?: string; + }, +): Promise { + const { owner, repo, prNumber } = params; + const log = childLogger({ service: "finding-dismissal", owner, repo, pr: prNumber }); + + const findings = await listPrFindingComments(octokit, owner, repo, prNumber); + let resolvedFindingIds = new Set(); + try { + resolvedFindingIds = await listResolvedFindingCommentIds( + octokit, + owner, + repo, + prNumber, + ); + } catch (err) { + log.warn({ err }, "Failed to load resolved review threads"); + } + const openFindings = findings.filter( + (finding) => + !isPrFindingDismissed(owner, repo, prNumber, finding.id) && + !resolvedFindingIds.has(finding.id), + ); + + if (openFindings.length > 0) { + log.info( + { + open: openFindings.length, + resolved: resolvedFindingIds.size, + total: findings.length, + }, + "Findings still open", + ); + return; + } + + if (findings.length === 0) return; + + const headSha = + params.headSha ?? + ( + await octokit.rest.pulls.get({ + owner, + repo, + pull_number: prNumber, + }) + ).data.head.sha; + + const { data: checkRuns } = await octokit.rest.checks.listForRef({ + owner, + repo, + ref: headSha, + check_name: CHECK_NAMES.PR_SCAN, + }); + const checkRun = checkRuns.check_runs.find((run) => run.name === CHECK_NAMES.PR_SCAN); + if (!checkRun) { + log.warn({ headSha }, "No PR scan check run found to update"); + return; + } + + await completeCheck(octokit, owner, repo, checkRun.id, "success", { + title: "PR scan passed", + summary: "All reported findings were reviewed and dismissed.", + }); + + await ensureLabels(octokit, owner, repo, PR_LABELS); + await setLabel(octokit, owner, repo, prNumber, LABEL_DEFS.PR_VERIFIED.name, PR_LABEL_NAMES); + log.info({ dismissed: findings.length }, "PR scan check cleared after dismissals"); + cancelDismissalReconcile(owner, repo, prNumber); +} + +const scheduledReconcileTimers = new Map[]>(); + +/** Poll GitHub for resolved threads when the review-thread webhook is not subscribed. */ +const RECONCILE_POLL_DELAYS_MS = [8_000, 25_000, 75_000, 180_000]; + +export function scheduleDismissalReconcile( + octokit: Octokit, + params: { owner: string; repo: string; prNumber: number; headSha: string }, +): void { + const key = `${params.owner}/${params.repo}#${params.prNumber}`; + const existing = scheduledReconcileTimers.get(key); + if (existing) { + for (const timer of existing) clearTimeout(timer); + } + + const timers = RECONCILE_POLL_DELAYS_MS.map((delayMs) => + setTimeout(() => { + void reconcilePrScanAfterDismissals(octokit, params).catch((err) => { + childLogger({ + service: "finding-dismissal", + owner: params.owner, + repo: params.repo, + pr: params.prNumber, + }).warn({ err, delayMs }, "Scheduled dismissal reconcile failed"); + }); + }, delayMs), + ); + + scheduledReconcileTimers.set(key, timers); +} + +export function cancelDismissalReconcile( + owner: string, + repo: string, + prNumber: number, +): void { + const key = `${owner}/${repo}#${prNumber}`; + const timers = scheduledReconcileTimers.get(key); + if (!timers) return; + for (const timer of timers) clearTimeout(timer); + scheduledReconcileTimers.delete(key); +} + +async function addEyesReaction( + octokit: Octokit, + owner: string, + repo: string, + commentId: number, + log: ReturnType, +): Promise { + try { + await octokit.rest.reactions.createForPullRequestReviewComment({ + owner, + repo, + comment_id: commentId, + content: "eyes", + }); + } catch (err) { + log.warn({ err, commentId }, "Failed to add eyes reaction to dismiss reply"); + } +} diff --git a/src/services/prFindingDismissals.ts b/src/services/prFindingDismissals.ts new file mode 100644 index 0000000..ee375aa --- /dev/null +++ b/src/services/prFindingDismissals.ts @@ -0,0 +1,34 @@ +import { queries } from "../lib/db.js"; + +export function dismissPrFinding(params: { + owner: string; + repo: string; + prNumber: number; + reviewCommentId: number; + dismissedBy: string; + headSha: string; +}): void { + queries.dismissPrFinding.run(params); +} + +export function isPrFindingDismissed( + owner: string, + repo: string, + prNumber: number, + reviewCommentId: number, +): boolean { + return !!queries.isPrFindingDismissed.get({ + owner, + repo, + prNumber, + reviewCommentId, + }); +} + +export function clearPrFindingDismissals( + owner: string, + repo: string, + prNumber: number, +): void { + queries.clearPrFindingDismissals.run({ owner, repo, prNumber }); +} diff --git a/src/services/prFindings.ts b/src/services/prFindings.ts new file mode 100644 index 0000000..6fe49dc --- /dev/null +++ b/src/services/prFindings.ts @@ -0,0 +1,268 @@ +import type { Octokit } from "octokit"; +import { MARKERS } from "../lib/types.js"; + +export interface PrReviewComment { + id: number; + body: string | null; + in_reply_to_id?: number | null; + path?: string; + line?: number | null; + user?: { login?: string; type?: string } | null; + author_association?: string | null; +} + +export function isPrFindingComment(body: string | null | undefined): boolean { + return !!body?.includes(MARKERS.PR_FINDING) && !body.includes(MARKERS.PR_FINDING_ACK); +} + +export async function listPrFindingComments( + octokit: Octokit, + owner: string, + repo: string, + prNumber: number, +): Promise { + const comments = await octokit.paginate(octokit.rest.pulls.listReviewComments, { + owner, + repo, + pull_number: prNumber, + per_page: 100, + }); + + return comments.filter( + (comment) => isPrFindingComment(comment.body) && comment.path && comment.line, + ); +} + +export async function getReviewComment( + octokit: Octokit, + owner: string, + repo: string, + commentId: number, +): Promise { + const { data } = await octokit.rest.pulls.getReviewComment({ + owner, + repo, + comment_id: commentId, + }); + return data; +} + +export async function resolveRootFindingComment( + octokit: Octokit, + owner: string, + repo: string, + comment: PrReviewComment, +): Promise { + let current = comment; + + for (let depth = 0; depth < 20; depth++) { + if (isPrFindingComment(current.body)) return current; + if (!current.in_reply_to_id) break; + current = await getReviewComment(octokit, owner, repo, current.in_reply_to_id); + } + + return null; +} + +export async function resolveFindingForUserReply( + octokit: Octokit, + owner: string, + repo: string, + prNumber: number, + comment: PrReviewComment, +): Promise { + const fromThread = await resolveRootFindingComment(octokit, owner, repo, comment); + if (fromThread) return fromThread; + + if (!comment.path) return null; + + const findings = await listPrFindingComments(octokit, owner, repo, prNumber); + if (!findings.length) return null; + + if (comment.line != null) { + const onSameLine = findings.find( + (finding) => finding.path === comment.path && finding.line === comment.line, + ); + if (onSameLine) return onSameLine; + } + + return findings.find((finding) => finding.path === comment.path) ?? null; +} + +export async function getReviewThreadIdForComment( + octokit: Octokit, + owner: string, + repo: string, + prNumber: number, + commentId: number, +): Promise { + const result = await octokit.graphql<{ + repository: { + pullRequest: { + reviewThreads: { + nodes: Array<{ + id: string; + comments: { + nodes: Array<{ databaseId: number | null }>; + }; + }>; + }; + } | null; + } | null; + }>( + `query($owner: String!, $repo: String!, $number: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + reviewThreads(first: 100) { + nodes { + id + comments(first: 100) { + nodes { + databaseId + } + } + } + } + } + } + }`, + { owner, repo, number: prNumber }, + ); + + const threads = result.repository?.pullRequest?.reviewThreads.nodes ?? []; + const thread = threads.find((candidate) => + candidate.comments.nodes.some((comment) => comment.databaseId === commentId), + ); + + return thread?.id ?? null; +} + +export async function listResolvedFindingCommentIds( + octokit: Octokit, + owner: string, + repo: string, + prNumber: number, +): Promise> { + const result = await octokit.graphql<{ + repository: { + pullRequest: { + reviewThreads: { + nodes: Array<{ + isResolved: boolean; + comments: { + nodes: Array<{ + databaseId: number | null; + body: string; + }>; + }; + }>; + }; + } | null; + } | null; + }>( + `query($owner: String!, $repo: String!, $number: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + reviewThreads(first: 100) { + nodes { + isResolved + comments(first: 100) { + nodes { + databaseId + body + } + } + } + } + } + } + }`, + { owner, repo, number: prNumber }, + ); + + const ids = new Set(); + const threads = result.repository?.pullRequest?.reviewThreads.nodes ?? []; + + for (const thread of threads) { + if (!thread.isResolved) continue; + const finding = thread.comments.nodes.find((comment) => + isPrFindingComment(comment.body), + ); + if (finding?.databaseId != null) ids.add(finding.databaseId); + } + + return ids; +} + +export async function resolveFindingForReviewThread( + octokit: Octokit, + owner: string, + repo: string, + prNumber: number, + thread: { + id?: string | number | null; + node_id?: string | null; + path?: string | null; + line?: number | null; + }, +): Promise { + const threadId = + thread.node_id ?? + (typeof thread.id === "string" && thread.id.startsWith("PRRT_") + ? thread.id + : null); + + if (threadId) { + const result = await octokit.graphql<{ + node: { + comments?: { + nodes: Array<{ + databaseId: number | null; + body: string; + path?: string | null; + line?: number | null; + }>; + }; + } | null; + }>( + `query($threadId: ID!) { + node(id: $threadId) { + ... on PullRequestReviewThread { + comments(first: 100) { + nodes { + databaseId + body + path + line + } + } + } + } + }`, + { threadId }, + ); + + const finding = result.node?.comments?.nodes.find((comment) => + isPrFindingComment(comment.body), + ); + if (finding?.databaseId != null) { + return { + id: finding.databaseId, + body: finding.body, + path: finding.path ?? undefined, + line: finding.line ?? undefined, + }; + } + } + + if (!thread.path) return null; + + const findings = await listPrFindingComments(octokit, owner, repo, prNumber); + return ( + findings.find( + (finding) => finding.path === thread.path && finding.line === thread.line, + ) ?? + findings.find((finding) => finding.path === thread.path) ?? + null + ); +} diff --git a/src/services/prScan.ts b/src/services/prScan.ts index f0b4199..650494a 100644 --- a/src/services/prScan.ts +++ b/src/services/prScan.ts @@ -1,11 +1,14 @@ import type { Octokit } from "octokit"; import type { PrFinding, RepoConfig } from "../lib/types.js"; +import { formatFindingPriority } from "../lib/findingPriority.js"; import { CHECK_NAMES, MARKERS, LABEL_DEFS } from "../lib/types.js"; import { evaluatePrScan } from "../lib/policy.js"; import { createInProgressCheck, completeCheck } from "./checkRuns.js"; import { deleteMarkerComment } from "./comments.js"; import { getGitHubToken } from "./githubToken.js"; import { scanPrLocally } from "./prScanner.js"; +import { clearPrFindingDismissals } from "./prFindingDismissals.js"; +import { scheduleDismissalReconcile } from "./findingDismissal.js"; import { clearLabels, ensureLabels, setLabel } from "./labels.js"; import { childLogger } from "../lib/logger.js"; @@ -67,6 +70,7 @@ export async function runPrScan( headSha, result.findings ?? [], ); + scheduleDismissalReconcile(octokit, { owner, repo, prNumber, headSha }); break; } @@ -100,7 +104,7 @@ function renderFindingsSummary(findings: PrFinding[]): string { const location = finding.file ? ` (${finding.file}${finding.line ? `:${finding.line}` : ""})` : ""; - return `${index + 1}. [${finding.severity.toUpperCase()}] ${finding.title}${location}\n${finding.recommendation}`; + return `${index + 1}. **${formatFindingPriority(finding.severity)}:** ${finding.title}${location}\n${finding.recommendation}`; }) .join("\n\n"); } @@ -113,6 +117,7 @@ async function replaceInlineFindingComments( headSha: string, findings: PrFinding[], ): Promise { + clearPrFindingDismissals(owner, repo, prNumber); await deleteInlineFindingComments(octokit, owner, repo, prNumber); const comments = findings @@ -169,11 +174,11 @@ function renderInlineFindingComment(finding: PrFinding): string { ); return `${MARKERS.PR_FINDING} -**[${finding.severity.toUpperCase()}] ${finding.title}** +**${formatFindingPriority(finding.severity)}:** ${finding.title} ${evidence} -Fix: ${recommendation}`; +${recommendation}`; } function compactSentence(value: string, maxLength: number): string { diff --git a/src/services/trustedContributor.ts b/src/services/trustedContributor.ts new file mode 100644 index 0000000..ab90d0c --- /dev/null +++ b/src/services/trustedContributor.ts @@ -0,0 +1,47 @@ +import type { Octokit } from "octokit"; + +const TRUSTED_ASSOCIATIONS = new Set([ + "OWNER", + "MEMBER", + "COLLABORATOR", + "CONTRIBUTOR", +]); + +const TRUSTED_PERMISSIONS = new Set(["admin", "maintain", "write"]); + +export function isTrustedAssociation(authorAssociation?: string | null): boolean { + return !!authorAssociation && TRUSTED_ASSOCIATIONS.has(authorAssociation); +} + +export async function isTrustedRepoContributor( + octokit: Octokit, + params: { + owner: string; + repo: string; + prNumber: number; + login: string; + authorAssociation?: string | null; + }, +): Promise { + const { owner, repo, prNumber, login, authorAssociation } = params; + + if (isTrustedAssociation(authorAssociation)) return true; + + const { data: pr } = await octokit.rest.pulls.get({ + owner, + repo, + pull_number: prNumber, + }); + if (pr.user?.login === login) return true; + + try { + const { data } = await octokit.rest.repos.getCollaboratorPermissionLevel({ + owner, + repo, + username: login, + }); + return TRUSTED_PERMISSIONS.has(data.permission); + } catch { + return false; + } +} From 3818955e441906d33c162f15713efd75a1d3a5e7 Mon Sep 17 00:00:00 2001 From: Ismail Pelaseyed Date: Wed, 20 May 2026 22:56:25 +0200 Subject: [PATCH 2/2] Restrict finding dismissals for first-time authors Only prior contributors or users with write-level access can dismiss findings; PR authorship alone no longer grants dismissal trust. --- .../__tests__/trustedContributor.test.ts | 85 ++++++++++++++++++- src/services/trustedContributor.ts | 22 +++-- 2 files changed, 95 insertions(+), 12 deletions(-) diff --git a/src/services/__tests__/trustedContributor.test.ts b/src/services/__tests__/trustedContributor.test.ts index 4366e47..40a9aeb 100644 --- a/src/services/__tests__/trustedContributor.test.ts +++ b/src/services/__tests__/trustedContributor.test.ts @@ -1,5 +1,8 @@ -import { describe, expect, it } from "vitest"; -import { isTrustedAssociation } from "../trustedContributor.js"; +import { describe, expect, it, vi } from "vitest"; +import { + isTrustedAssociation, + isTrustedRepoContributor, +} from "../trustedContributor.js"; describe("isTrustedAssociation", () => { it("trusts repository contributors and maintainers", () => { @@ -9,9 +12,85 @@ describe("isTrustedAssociation", () => { expect(isTrustedAssociation("CONTRIBUTOR")).toBe(true); }); - it("does not trust drive-by or first-time authors", () => { + it("does not trust first-time authors", () => { expect(isTrustedAssociation("NONE")).toBe(false); expect(isTrustedAssociation("FIRST_TIMER")).toBe(false); + expect(isTrustedAssociation("FIRST_TIME_CONTRIBUTOR")).toBe(false); expect(isTrustedAssociation(undefined)).toBe(false); }); }); + +describe("isTrustedRepoContributor", () => { + function mockOctokit(options?: { + permission?: string; + permissionError?: boolean; + contributors?: Array<{ login?: string }>; + }) { + return { + rest: { + repos: { + getCollaboratorPermissionLevel: vi.fn().mockImplementation(() => { + if (options?.permissionError) throw new Error("not a collaborator"); + return Promise.resolve({ + data: { permission: options?.permission ?? "read" }, + }); + }), + listContributors: vi.fn(), + }, + }, + paginate: vi.fn().mockResolvedValue(options?.contributors ?? []), + } as any; + } + + it("does not trust a first-time PR author by ownership alone", async () => { + const octokit = mockOctokit({ + permission: "read", + contributors: [], + }); + + await expect( + isTrustedRepoContributor(octokit, { + owner: "acme", + repo: "repo", + prNumber: 1, + login: "first-timer", + authorAssociation: "FIRST_TIME_CONTRIBUTOR", + }), + ).resolves.toBe(false); + + expect(octokit.rest.repos.getCollaboratorPermissionLevel).toHaveBeenCalled(); + expect(octokit.paginate).toHaveBeenCalled(); + }); + + it("trusts prior contributors even without write permission", async () => { + const octokit = mockOctokit({ + permission: "read", + contributors: [{ login: "prior-contributor" }], + }); + + await expect( + isTrustedRepoContributor(octokit, { + owner: "acme", + repo: "repo", + prNumber: 1, + login: "prior-contributor", + authorAssociation: "FIRST_TIME_CONTRIBUTOR", + }), + ).resolves.toBe(true); + }); + + it("trusts users with write-level repository permissions", async () => { + const octokit = mockOctokit({ permission: "write" }); + + await expect( + isTrustedRepoContributor(octokit, { + owner: "acme", + repo: "repo", + prNumber: 1, + login: "maintainer", + }), + ).resolves.toBe(true); + + expect(octokit.paginate).not.toHaveBeenCalled(); + }); +}); diff --git a/src/services/trustedContributor.ts b/src/services/trustedContributor.ts index ab90d0c..2b6cd80 100644 --- a/src/services/trustedContributor.ts +++ b/src/services/trustedContributor.ts @@ -23,24 +23,28 @@ export async function isTrustedRepoContributor( authorAssociation?: string | null; }, ): Promise { - const { owner, repo, prNumber, login, authorAssociation } = params; + const { owner, repo, login, authorAssociation } = params; if (isTrustedAssociation(authorAssociation)) return true; - const { data: pr } = await octokit.rest.pulls.get({ - owner, - repo, - pull_number: prNumber, - }); - if (pr.user?.login === login) return true; - try { const { data } = await octokit.rest.repos.getCollaboratorPermissionLevel({ owner, repo, username: login, }); - return TRUSTED_PERMISSIONS.has(data.permission); + if (TRUSTED_PERMISSIONS.has(data.permission)) return true; + } catch { + // Public contributors are not always collaborators, so fall through. + } + + try { + const contributors = await octokit.paginate(octokit.rest.repos.listContributors, { + owner, + repo, + per_page: 100, + }); + return contributors.some((contributor) => contributor.login === login); } catch { return false; }