diff --git a/src/lib/db.ts b/src/lib/db.ts index 486a018..a9c4ef0 100644 --- a/src/lib/db.ts +++ b/src/lib/db.ts @@ -46,6 +46,7 @@ db.exec(` repo TEXT NOT NULL, pr_number INTEGER NOT NULL, review_comment_id INTEGER NOT NULL, + finding_fingerprint TEXT, dismissed_by TEXT NOT NULL, head_sha TEXT NOT NULL, dismissed_at TEXT NOT NULL DEFAULT (datetime('now')), @@ -53,6 +54,17 @@ db.exec(` ); `); +const dismissalColumns = db + .prepare("PRAGMA table_info(pr_finding_dismissals)") + .all() as Array<{ name: string }>; +if (!dismissalColumns.some((column) => column.name === "finding_fingerprint")) { + db.exec("ALTER TABLE pr_finding_dismissals ADD COLUMN finding_fingerprint TEXT"); +} +db.exec(` + CREATE INDEX IF NOT EXISTS idx_pr_finding_dismissals_fingerprint + ON pr_finding_dismissals (owner, repo, pr_number, finding_fingerprint) +`); + logger.info({ path: env.dbPath }, "Database initialized"); export const queries: Record = { @@ -132,15 +144,16 @@ export const queries: Record = { dismissPrFinding: db.prepare(` INSERT INTO pr_finding_dismissals ( - owner, repo, pr_number, review_comment_id, dismissed_by, head_sha + owner, repo, pr_number, review_comment_id, finding_fingerprint, dismissed_by, head_sha ) VALUES ( - @owner, @repo, @prNumber, @reviewCommentId, @dismissedBy, @headSha + @owner, @repo, @prNumber, @reviewCommentId, @findingFingerprint, @dismissedBy, @headSha ) ON CONFLICT(owner, repo, pr_number, review_comment_id) DO UPDATE SET - dismissed_by = @dismissedBy, - head_sha = @headSha, - dismissed_at = datetime('now') + finding_fingerprint = @findingFingerprint, + dismissed_by = @dismissedBy, + head_sha = @headSha, + dismissed_at = datetime('now') `), isPrFindingDismissed: db.prepare(` @@ -153,6 +166,17 @@ export const queries: Record = { LIMIT 1 `), + isPrFindingFingerprintDismissed: db.prepare(` + SELECT 1 + FROM pr_finding_dismissals + WHERE owner = @owner + AND repo = @repo + AND pr_number = @prNumber + AND finding_fingerprint = @findingFingerprint + AND head_sha = @headSha + LIMIT 1 + `), + clearPrFindingDismissals: db.prepare(` DELETE FROM pr_finding_dismissals WHERE owner = @owner AND repo = @repo AND pr_number = @prNumber diff --git a/src/lib/types.ts b/src/lib/types.ts index b343c7e..a47faff 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -1,3 +1,5 @@ +export const SUPERAGENT_URL = "https://superagent.sh"; + export const CHECK_NAMES = { PR_SCAN: "Security scan", CONTRIBUTOR_TRUST: "Contributor trust", @@ -64,7 +66,6 @@ export interface ContributorResult { verdict?: string; confidence?: string; name?: string; - url?: string; threats?: Array<{ type: string; detail: string; severity: string }>; sub_scores?: { identity?: number; diff --git a/src/services/__tests__/checkRuns.test.ts b/src/services/__tests__/checkRuns.test.ts new file mode 100644 index 0000000..5732f8e --- /dev/null +++ b/src/services/__tests__/checkRuns.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, it, vi } from "vitest"; +import { SUPERAGENT_URL } from "../../lib/types.js"; +import { completeCheck, createInProgressCheck } from "../checkRuns.js"; + +describe("checkRuns", () => { + it("sets Superagent as the check run details URL", async () => { + const octokit = { + rest: { + checks: { + create: vi.fn().mockResolvedValue({ data: { id: 42 } }), + update: vi.fn().mockResolvedValue({}), + }, + }, + }; + + const checkRunId = await createInProgressCheck( + octokit as never, + "acme", + "repo", + "abc123", + "Security scan", + ); + + expect(checkRunId).toBe(42); + expect(octokit.rest.checks.create).toHaveBeenCalledWith( + expect.objectContaining({ + details_url: SUPERAGENT_URL, + }), + ); + + await completeCheck(octokit as never, "acme", "repo", checkRunId, "success", { + title: "PR scan passed", + summary: "No suspicious PR changes were detected.", + }); + + expect(octokit.rest.checks.update).toHaveBeenCalledWith( + expect.objectContaining({ + details_url: SUPERAGENT_URL, + }), + ); + }); +}); diff --git a/src/services/__tests__/comments.test.ts b/src/services/__tests__/comments.test.ts index fadebe0..a1eaeb9 100644 --- a/src/services/__tests__/comments.test.ts +++ b/src/services/__tests__/comments.test.ts @@ -55,7 +55,6 @@ describe("renderContributorTrustComment", () => { score: 25, verdict: "suspicious", name: "sketchy-user", - url: "https://superagent.sh/contributor/sketchy-user", threats: [ { type: "new_account", severity: "high", detail: "Account created less than 30 days ago" }, ], @@ -80,7 +79,7 @@ describe("renderContributorTrustComment", () => { expect(body).toContain("| Behavior | 30 |"); expect(body).toContain("| Content | 40 |"); expect(body).not.toContain("| Graph |"); - expect(body).toContain("[Full profile](https://superagent.sh/contributor/sketchy-user?details=true)"); + expect(body).toContain("Analyzed by [Superagent]"); }); it("renders without threats section when empty", () => { @@ -107,18 +106,6 @@ describe("renderContributorTrustComment", () => { expect(body).not.toContain("| Graph |"); }); - it("omits full profile link when url is missing", () => { - const result: ContributorResult = { - score: 40, - verdict: "caution", - name: "some-user", - }; - const body = renderContributorTrustComment(result); - - expect(body).not.toContain("Full profile"); - expect(body).toContain("Analyzed by [Superagent]"); - }); - it("uses correct emoji for each verdict", () => { expect(renderContributorTrustComment({ score: 30, verdict: "caution", name: "a" })) .toContain("\u26A0\uFE0F"); diff --git a/src/services/__tests__/findingDismissal.test.ts b/src/services/__tests__/findingDismissal.test.ts index 6cdafbf..3f74547 100644 --- a/src/services/__tests__/findingDismissal.test.ts +++ b/src/services/__tests__/findingDismissal.test.ts @@ -2,9 +2,12 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { CHECK_NAMES, LABEL_DEFS, MARKERS } from "../../lib/types.js"; const dismissedFindingIds = vi.hoisted(() => new Set()); +const dismissPrFindingMock = vi.hoisted(() => vi.fn()); vi.mock("../prFindingDismissals.js", () => ({ - dismissPrFinding: ({ reviewCommentId }: { reviewCommentId: number }) => { + dismissPrFinding: (params: { reviewCommentId: number }) => { + dismissPrFindingMock(params); + const { reviewCommentId } = params; dismissedFindingIds.add(reviewCommentId); }, isPrFindingDismissed: ( @@ -47,6 +50,7 @@ import { isTrustedRepoContributor } from "../trustedContributor.js"; import { handleFindingThreadResolved, handleFindingReply, + persistReviewedFindingDismissals, reconcilePrScanAfterDismissals, } from "../findingDismissal.js"; @@ -69,6 +73,8 @@ describe("handleFindingReply", () => { body: `${MARKERS.PR_FINDING}\n**P2:** Sandbox scopes`, path: "pkg.ts", line: 1, + commit_id: "finding-sha", + user: { login: "superagent-security[bot]", type: "Bot" }, }, ], }); @@ -111,6 +117,12 @@ describe("handleFindingReply", () => { labels: expect.arrayContaining([LABEL_DEFS.PR_VERIFIED.name]), }), ); + expect(dismissPrFindingMock).toHaveBeenCalledWith( + expect.objectContaining({ + reviewCommentId: 10, + headSha: "finding-sha", + }), + ); }); it("uses permissive model evaluation for trusted contributor replies", async () => { @@ -200,7 +212,13 @@ describe("handleFindingThreadResolved", () => { vi.mocked(isTrustedRepoContributor).mockResolvedValueOnce(true); const octokit = mockOctokit({ findingComments: [ - { id: 10, body: `${MARKERS.PR_FINDING}\n**P2:** one`, path: "a.ts", line: 1 }, + { + id: 10, + body: `${MARKERS.PR_FINDING}\n**P2:** one`, + path: "a.ts", + line: 1, + user: { login: "superagent-security[bot]", type: "Bot" }, + }, ], }); @@ -249,8 +267,20 @@ describe("reconcilePrScanAfterDismissals", () => { 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 }, + { + id: 10, + body: `${MARKERS.PR_FINDING}\n**P2:** one`, + path: "a.ts", + line: 1, + user: { login: "superagent-security[bot]", type: "Bot" }, + }, + { + id: 20, + body: `${MARKERS.PR_FINDING}\n**P1:** two`, + path: "b.ts", + line: 2, + user: { login: "superagent-security[bot]", type: "Bot" }, + }, ], }); @@ -265,13 +295,57 @@ describe("reconcilePrScanAfterDismissals", () => { }); }); +describe("persistReviewedFindingDismissals", () => { + beforeEach(() => { + vi.clearAllMocks(); + dismissedFindingIds.clear(); + }); + + it("records reviewed findings against the review comment commit SHA", async () => { + const octokit = mockOctokit({ + findingComments: [ + { + id: 10, + body: `${MARKERS.PR_FINDING}\n**P2:** one`, + path: "a.ts", + line: 1, + commit_id: "comment-sha", + user: { login: "superagent-security[bot]", type: "Bot" }, + }, + { + id: 11, + body: MARKERS.PR_FINDING_ACK, + in_reply_to_id: 10, + user: { login: "superagent-security[bot]", type: "Bot" }, + }, + ], + }); + + await persistReviewedFindingDismissals(octokit, { + owner: "acme", + repo: "repo", + prNumber: 12, + headSha: "current-sha", + }); + + expect(dismissPrFindingMock).toHaveBeenCalledWith( + expect.objectContaining({ + reviewCommentId: 10, + headSha: "comment-sha", + }), + ); + }); +}); + function mockOctokit(options?: { findingComments?: Array<{ id: number; body: string; - path: string; - line: number; + path?: string; + line?: number; + commit_id?: string; in_reply_to_id?: number; + user?: { login?: string; type?: string }; }>; }) { const findingComments = options?.findingComments ?? [ @@ -280,6 +354,7 @@ function mockOctokit(options?: { body: `${MARKERS.PR_FINDING}\n**P2:** Sandbox scopes`, path: "pkg.ts", line: 1, + user: { login: "superagent-security[bot]", type: "Bot" }, }, ]; const commentById = new Map( diff --git a/src/services/__tests__/prFindings.test.ts b/src/services/__tests__/prFindings.test.ts index 7403b46..d7d68ca 100644 --- a/src/services/__tests__/prFindings.test.ts +++ b/src/services/__tests__/prFindings.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it, vi } from "vitest"; import { MARKERS } from "../../lib/types.js"; import { isPrFindingComment, + listAcknowledgedFindingCommentIds, resolveFindingForUserReply, resolveRootFindingComment, } from "../prFindings.js"; @@ -32,6 +33,7 @@ describe("resolveRootFindingComment", () => { id: 10, body: `${MARKERS.PR_FINDING}\n**P2:** root`, in_reply_to_id: null, + user: { login: "superagent-security[bot]", type: "Bot" }, }, }), }, @@ -48,6 +50,45 @@ describe("resolveRootFindingComment", () => { }); }); +describe("listAcknowledgedFindingCommentIds", () => { + it("only trusts Superagent-authored acknowledgment markers", async () => { + const listReviewComments = vi.fn(); + const octokit = { + paginate: vi.fn().mockResolvedValue([ + { + id: 11, + body: MARKERS.PR_FINDING_ACK, + in_reply_to_id: 10, + user: { login: "malicious-user", type: "User" }, + }, + { + id: 12, + body: MARKERS.PR_FINDING_ACK, + in_reply_to_id: 20, + user: { login: "other-bot[bot]", type: "Bot" }, + }, + { + id: 13, + body: MARKERS.PR_FINDING_ACK, + in_reply_to_id: 30, + user: { login: "superagent-security-dev[bot]", type: "Bot" }, + }, + { + id: 14, + body: MARKERS.PR_FINDING_ACK, + in_reply_to_id: 40, + user: { login: "superagent-security[bot]", type: "Bot" }, + }, + ]), + rest: { pulls: { listReviewComments } }, + } as any; + + const ids = await listAcknowledgedFindingCommentIds(octokit, "acme", "repo", 12); + + expect([...ids]).toEqual([30, 40]); + }); +}); + describe("resolveFindingForUserReply", () => { it("matches a finding on the same file line when the reply has no in_reply_to_id", async () => { const octokit = { @@ -57,6 +98,7 @@ describe("resolveFindingForUserReply", () => { body: `${MARKERS.PR_FINDING}\n**P0:** issue`, path: ".github/workflows/test.yml", line: 17, + user: { login: "superagent-security[bot]", type: "Bot" }, }, ]), rest: { pulls: { listReviewComments: vi.fn() } }, @@ -71,4 +113,28 @@ describe("resolveFindingForUserReply", () => { expect(finding?.id).toBe(10); }); + + it("ignores forged finding comments on the same file line", async () => { + const octokit = { + paginate: vi.fn().mockResolvedValue([ + { + id: 10, + body: `${MARKERS.PR_FINDING}\n**P0:** forged`, + path: ".github/workflows/test.yml", + line: 17, + user: { login: "malicious-user", type: "User" }, + }, + ]), + 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).toBeNull(); + }); }); diff --git a/src/services/__tests__/prScan.test.ts b/src/services/__tests__/prScan.test.ts index 8bfa0e9..5e0c101 100644 --- a/src/services/__tests__/prScan.test.ts +++ b/src/services/__tests__/prScan.test.ts @@ -2,8 +2,11 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { CHECK_NAMES, DEFAULT_CONFIG } from "../../lib/types.js"; import { runPrScan } from "../prScan.js"; +const isPrFindingFingerprintDismissedMock = vi.hoisted(() => vi.fn()); + vi.mock("../prFindingDismissals.js", () => ({ clearPrFindingDismissals: vi.fn(), + isPrFindingFingerprintDismissed: isPrFindingFingerprintDismissedMock, })); vi.mock("../../lib/logger.js", () => ({ @@ -14,10 +17,16 @@ vi.mock("../../lib/logger.js", () => ({ }), })); +vi.mock("../findingDismissal.js", () => ({ + persistReviewedFindingDismissals: vi.fn().mockResolvedValue([]), + scheduleDismissalReconcile: vi.fn(), +})); + describe("runPrScan", () => { beforeEach(() => { vi.clearAllMocks(); vi.unstubAllGlobals(); + isPrFindingFingerprintDismissedMock.mockReturnValue(false); }); it("comments and flags the PR when local scanner returns findings", async () => { @@ -92,6 +101,55 @@ describe("runPrScan", () => { expect(body).not.toContain("Recommended fix"); }); + it("does not recreate comments for previously dismissed findings", async () => { + isPrFindingFingerprintDismissedMock.mockReturnValue(true); + stubScanFetch({ + findings: [ + { + category: "malicious_intent", + severity: "high", + title: "Suspicious lifecycle hook", + file: "package.json", + line: 1, + evidence: "postinstall executes a remote script.", + recommendation: "Remove the hook and vendor reviewed setup code.", + short_evidence: "A new postinstall hook executes a remote script.", + short_recommendation: "Remove the lifecycle hook or replace it with reviewed local setup code.", + }, + ], + }); + const octokit = mockOctokit(); + + await runPrScan(octokit, { + owner: "acme", + repo: "repo", + prNumber: 12, + headSha: "abc123", + config: DEFAULT_CONFIG, + }); + + expect(octokit.rest.checks.update).toHaveBeenCalledWith( + expect.objectContaining({ + conclusion: "success", + output: expect.objectContaining({ + summary: "All detected findings were previously reviewed and dismissed.", + }), + }), + ); + expect(octokit.rest.issues.setLabels).toHaveBeenCalledWith( + expect.objectContaining({ labels: ["keep", "pr:verified"] }), + ); + expect(octokit.rest.pulls.createReview).not.toHaveBeenCalled(); + expect(octokit.rest.pulls.deleteReviewComment).not.toHaveBeenCalled(); + expect(isPrFindingFingerprintDismissedMock).toHaveBeenCalledWith( + "acme", + "repo", + 12, + expect.any(String), + "abc123", + ); + }); + it("verifies the PR and removes stale comments when no findings are found", async () => { stubScanFetch({ findings: [] }); const octokit = mockOctokit([{ id: 99, body: " old comment" }]); diff --git a/src/services/checkRuns.ts b/src/services/checkRuns.ts index 92d2952..f5cc50a 100644 --- a/src/services/checkRuns.ts +++ b/src/services/checkRuns.ts @@ -1,4 +1,5 @@ import type { Octokit } from "octokit"; +import { SUPERAGENT_URL } from "../lib/types.js"; export interface CheckOutput { title: string; @@ -31,6 +32,7 @@ export async function createInProgressCheck( head_sha: headSha, status: "in_progress", started_at: new Date().toISOString(), + details_url: SUPERAGENT_URL, }); return data.id; } @@ -50,6 +52,7 @@ export async function completeCheck( status: "completed", conclusion, completed_at: new Date().toISOString(), + details_url: SUPERAGENT_URL, output, }); } diff --git a/src/services/comments.ts b/src/services/comments.ts index f6e8367..9afa94c 100644 --- a/src/services/comments.ts +++ b/src/services/comments.ts @@ -1,7 +1,7 @@ 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"; +import { MARKERS, SUPERAGENT_URL } from "../lib/types.js"; export async function upsertComment( octokit: Octokit, @@ -80,7 +80,7 @@ export function renderPrScanComment( body += `- **Recommended fix:** ${finding.recommendation}\n\n`; } - body += `Analyzed by [Superagent](https://superagent.sh)`; + body += `Analyzed by [Superagent](${SUPERAGENT_URL})`; return body; } @@ -127,11 +127,7 @@ export function renderContributorTrustComment( body += `\n`; body += `\n\n`; - body += `Analyzed by [Superagent](https://superagent.sh)`; - if (result.url) { - body += ` \u00b7 [Full profile](${result.url}?details=true)`; - } - body += ``; + body += `Analyzed by [Superagent](${SUPERAGENT_URL})`; return body; } diff --git a/src/services/findingDismissal.ts b/src/services/findingDismissal.ts index 7103c87..82d761f 100644 --- a/src/services/findingDismissal.ts +++ b/src/services/findingDismissal.ts @@ -6,6 +6,8 @@ import { completeCheck } from "./checkRuns.js"; import { loadConfig } from "./config.js"; import { getReviewThreadIdForComment, + fingerprintPrFindingCommentBody, + listAcknowledgedFindingCommentIds, listPrFindingComments, listResolvedFindingCommentIds, resolveFindingForUserReply, @@ -95,6 +97,8 @@ export async function handleFindingReply( repo, prNumber, rootFindingId: rootFinding.id, + rootFindingBody: rootFinding.body, + rootFindingCommitId: rootFinding.commit_id, replyToCommentId: comment.id, dismissedBy: login || "unknown", acknowledgment: "Got it, thanks for the context.", @@ -109,6 +113,8 @@ export async function handleFindingReply( repo, prNumber, rootFindingId: rootFinding.id, + rootFindingBody: rootFinding.body, + rootFindingCommitId: rootFinding.commit_id, replyToCommentId: comment.id, dismissedBy: login || "unknown", acknowledgment: evaluation.acknowledgment, @@ -202,6 +208,7 @@ export async function handleFindingThreadResolved( repo, prNumber, reviewCommentId: rootFinding.id, + findingFingerprint: fingerprintPrFindingCommentBody(rootFinding.body), dismissedBy: login || "unknown", headSha: pr.head.sha, }); @@ -225,13 +232,24 @@ async function dismissFinding( repo: string; prNumber: number; rootFindingId: number; + rootFindingBody: string | null; + rootFindingCommitId?: string | null; replyToCommentId: number; dismissedBy: string; acknowledgment: string; }, ): Promise { - const { owner, repo, prNumber, rootFindingId, replyToCommentId, dismissedBy, acknowledgment } = - params; + const { + owner, + repo, + prNumber, + rootFindingId, + rootFindingBody, + rootFindingCommitId, + replyToCommentId, + dismissedBy, + acknowledgment, + } = params; const { data: pr } = await octokit.rest.pulls.get({ owner, @@ -244,8 +262,9 @@ async function dismissFinding( repo, prNumber, reviewCommentId: rootFindingId, + findingFingerprint: fingerprintPrFindingCommentBody(rootFindingBody), dismissedBy, - headSha: pr.head.sha, + headSha: rootFindingCommitId ?? pr.head.sha, }); await postFindingReply(octokit, { @@ -336,30 +355,32 @@ export async function reconcilePrScanAfterDismissals( ): Promise { const { owner, repo, prNumber } = params; const log = childLogger({ service: "finding-dismissal", owner, repo, pr: prNumber }); + const currentHeadSha = + params.headSha ?? + ( + await octokit.rest.pulls.get({ + owner, + repo, + pull_number: prNumber, + }) + ).data.head.sha; + + const findings = await persistReviewedFindingDismissals(octokit, { + owner, + repo, + prNumber, + headSha: currentHeadSha, + }); + if (findings.length === 0) return; - 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), + (finding) => !isPrFindingDismissed(owner, repo, prNumber, finding.id), ); if (openFindings.length > 0) { log.info( { open: openFindings.length, - resolved: resolvedFindingIds.size, total: findings.length, }, "Findings still open", @@ -367,27 +388,15 @@ export async function reconcilePrScanAfterDismissals( 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, + ref: currentHeadSha, 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"); + log.warn({ headSha: currentHeadSha }, "No PR scan check run found to update"); return; } @@ -402,6 +411,59 @@ export async function reconcilePrScanAfterDismissals( cancelDismissalReconcile(owner, repo, prNumber); } +export async function persistReviewedFindingDismissals( + octokit: Octokit, + params: { + owner: string; + repo: string; + prNumber: number; + headSha: string; + }, +): Promise { + const { owner, repo, prNumber, headSha } = params; + const log = childLogger({ service: "finding-dismissal", owner, repo, pr: prNumber }); + const findings = await listPrFindingComments(octokit, owner, repo, prNumber); + if (findings.length === 0) return findings; + + const dismissedFindingIds = new Set(); + try { + for (const id of await listAcknowledgedFindingCommentIds(octokit, owner, repo, prNumber)) { + dismissedFindingIds.add(id); + } + } catch (err) { + log.warn({ err }, "Failed to load acknowledged finding replies"); + } + + let resolvedFindingIds = new Set(); + try { + resolvedFindingIds = await listResolvedFindingCommentIds( + octokit, + owner, + repo, + prNumber, + ); + } catch (err) { + log.warn({ err }, "Failed to load resolved review threads"); + } + + for (const finding of findings) { + if (!dismissedFindingIds.has(finding.id) && !resolvedFindingIds.has(finding.id)) { + continue; + } + dismissPrFinding({ + owner, + repo, + prNumber, + reviewCommentId: finding.id, + findingFingerprint: fingerprintPrFindingCommentBody(finding.body), + dismissedBy: "reviewed-thread", + headSha: finding.commit_id ?? headSha, + }); + } + + return findings; +} + const scheduledReconcileTimers = new Map[]>(); /** Poll GitHub for resolved threads when the review-thread webhook is not subscribed. */ diff --git a/src/services/prFindingDismissals.ts b/src/services/prFindingDismissals.ts index ee375aa..92d68c3 100644 --- a/src/services/prFindingDismissals.ts +++ b/src/services/prFindingDismissals.ts @@ -5,10 +5,14 @@ export function dismissPrFinding(params: { repo: string; prNumber: number; reviewCommentId: number; + findingFingerprint?: string | null; dismissedBy: string; headSha: string; }): void { - queries.dismissPrFinding.run(params); + queries.dismissPrFinding.run({ + ...params, + findingFingerprint: params.findingFingerprint ?? null, + }); } export function isPrFindingDismissed( @@ -25,6 +29,22 @@ export function isPrFindingDismissed( }); } +export function isPrFindingFingerprintDismissed( + owner: string, + repo: string, + prNumber: number, + findingFingerprint: string, + headSha: string, +): boolean { + return !!queries.isPrFindingFingerprintDismissed.get({ + owner, + repo, + prNumber, + findingFingerprint, + headSha, + }); +} + export function clearPrFindingDismissals( owner: string, repo: string, diff --git a/src/services/prFindings.ts b/src/services/prFindings.ts index 6fe49dc..9f5fb93 100644 --- a/src/services/prFindings.ts +++ b/src/services/prFindings.ts @@ -1,10 +1,19 @@ import type { Octokit } from "octokit"; +import { createHash } from "node:crypto"; import { MARKERS } from "../lib/types.js"; +const SUPERAGENT_BOT_LOGINS = new Set([ + "superagent-security", + "superagent-security[bot]", + "superagent-security-dev", + "superagent-security-dev[bot]", +]); + export interface PrReviewComment { id: number; body: string | null; in_reply_to_id?: number | null; + commit_id?: string | null; path?: string; line?: number | null; user?: { login?: string; type?: string } | null; @@ -15,6 +24,39 @@ export function isPrFindingComment(body: string | null | undefined): boolean { return !!body?.includes(MARKERS.PR_FINDING) && !body.includes(MARKERS.PR_FINDING_ACK); } +function isSuperagentFindingComment(comment: { + body: string | null; + user?: PrReviewComment["user"]; +}): boolean { + return isPrFindingComment(comment.body) && isSuperagentBot(comment.user); +} + +const FINGERPRINT_MARKER_RE = + //i; + +export function fingerprintPrFindingCommentBody( + body: string | null | undefined, +): string | undefined { + if (!body) return undefined; + + const existing = body.match(FINGERPRINT_MARKER_RE)?.[1]; + if (existing) return existing; + + const normalized = body + .replace(FINGERPRINT_MARKER_RE, "") + .replace(MARKERS.PR_FINDING, "") + .replace(/\s+/g, " ") + .trim() + .toLowerCase(); + if (!normalized) return undefined; + + return createHash("sha256").update(normalized).digest("hex"); +} + +export function appendPrFindingFingerprint(body: string, fingerprint: string): string { + return `${body}\n\n`; +} + export async function listPrFindingComments( octokit: Octokit, owner: string, @@ -29,10 +71,41 @@ export async function listPrFindingComments( }); return comments.filter( - (comment) => isPrFindingComment(comment.body) && comment.path && comment.line, + (comment) => isSuperagentFindingComment(comment) && comment.path && comment.line, ); } +export async function listAcknowledgedFindingCommentIds( + 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, + }); + + const ids = new Set(); + for (const comment of comments) { + if (!comment.body?.includes(MARKERS.PR_FINDING_ACK)) continue; + if (!isSuperagentBot(comment.user)) continue; + if (comment.in_reply_to_id != null) ids.add(comment.in_reply_to_id); + } + + return ids; +} + +function isSuperagentBot(user: PrReviewComment["user"]): boolean { + return isSuperagentBotLogin(user?.login) && user?.type === "Bot"; +} + +function isSuperagentBotLogin(login: string | null | undefined): boolean { + return !!login && SUPERAGENT_BOT_LOGINS.has(login); +} + export async function getReviewComment( octokit: Octokit, owner: string, @@ -56,7 +129,7 @@ export async function resolveRootFindingComment( let current = comment; for (let depth = 0; depth < 20; depth++) { - if (isPrFindingComment(current.body)) return current; + if (isSuperagentFindingComment(current)) return current; if (!current.in_reply_to_id) break; current = await getReviewComment(octokit, owner, repo, current.in_reply_to_id); } @@ -153,6 +226,7 @@ export async function listResolvedFindingCommentIds( nodes: Array<{ databaseId: number | null; body: string; + author?: { login?: string | null } | null; }>; }; }>; @@ -170,6 +244,9 @@ export async function listResolvedFindingCommentIds( nodes { databaseId body + author { + login + } } } } @@ -186,7 +263,7 @@ export async function listResolvedFindingCommentIds( for (const thread of threads) { if (!thread.isResolved) continue; const finding = thread.comments.nodes.find((comment) => - isPrFindingComment(comment.body), + isPrFindingComment(comment.body) && isSuperagentBotLogin(comment.author?.login), ); if (finding?.databaseId != null) ids.add(finding.databaseId); } @@ -221,6 +298,7 @@ export async function resolveFindingForReviewThread( body: string; path?: string | null; line?: number | null; + author?: { login?: string | null } | null; }>; }; } | null; @@ -234,6 +312,9 @@ export async function resolveFindingForReviewThread( body path line + author { + login + } } } } @@ -243,7 +324,7 @@ export async function resolveFindingForReviewThread( ); const finding = result.node?.comments?.nodes.find((comment) => - isPrFindingComment(comment.body), + isPrFindingComment(comment.body) && isSuperagentBotLogin(comment.author?.login), ); if (finding?.databaseId != null) { return { diff --git a/src/services/prScan.ts b/src/services/prScan.ts index 650494a..bf50a94 100644 --- a/src/services/prScan.ts +++ b/src/services/prScan.ts @@ -7,10 +7,20 @@ 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 { + clearPrFindingDismissals, + isPrFindingFingerprintDismissed, +} from "./prFindingDismissals.js"; +import { + persistReviewedFindingDismissals, + scheduleDismissalReconcile, +} from "./findingDismissal.js"; import { clearLabels, ensureLabels, setLabel } from "./labels.js"; import { childLogger } from "../lib/logger.js"; +import { + appendPrFindingFingerprint, + fingerprintPrFindingCommentBody, +} from "./prFindings.js"; const PR_LABELS = [LABEL_DEFS.PR_VERIFIED, LABEL_DEFS.PR_FLAGGED]; const PR_LABEL_NAMES = PR_LABELS.map((l) => l.name); @@ -55,10 +65,26 @@ export async function runPrScan( switch (status) { case "review": { + await persistReviewedFindingDismissals(octokit, { owner, repo, prNumber, headSha }); + const findings = result.findings ?? []; + const openFindings = findings.filter( + (finding) => !isFindingDismissed(owner, repo, prNumber, headSha, finding), + ); + + if (!openFindings.length) { + await completeCheck(octokit, owner, repo, checkRunId, "success", { + title: "PR scan passed", + summary: "All detected findings were previously reviewed and dismissed.", + }); + await setLabel(octokit, owner, repo, prNumber, LABEL_DEFS.PR_VERIFIED.name, PR_LABEL_NAMES); + await deleteMarkerComment(octokit, owner, repo, prNumber, MARKERS.PR_SCAN); + break; + } + await completeCheck(octokit, owner, repo, checkRunId, "action_required", { title: "PR requires security review", - summary: `${result.findings?.length ?? 0} security concern(s) detected.`, - text: renderFindingsSummary(result.findings ?? []), + summary: `${openFindings.length} security concern(s) detected.`, + text: renderFindingsSummary(openFindings), }); await setLabel(octokit, owner, repo, prNumber, LABEL_DEFS.PR_FLAGGED.name, PR_LABEL_NAMES); await deleteMarkerComment(octokit, owner, repo, prNumber, MARKERS.PR_SCAN); @@ -68,7 +94,7 @@ export async function runPrScan( repo, prNumber, headSha, - result.findings ?? [], + openFindings, ); scheduleDismissalReconcile(octokit, { owner, repo, prNumber, headSha }); break; @@ -82,6 +108,7 @@ export async function runPrScan( await setLabel(octokit, owner, repo, prNumber, LABEL_DEFS.PR_VERIFIED.name, PR_LABEL_NAMES); await deleteMarkerComment(octokit, owner, repo, prNumber, MARKERS.PR_SCAN); await deleteInlineFindingComments(octokit, owner, repo, prNumber); + clearPrFindingDismissals(owner, repo, prNumber); break; } @@ -98,6 +125,21 @@ export async function runPrScan( } } +function isFindingDismissed( + owner: string, + repo: string, + prNumber: number, + headSha: string, + finding: PrFinding, +): boolean { + const fingerprint = fingerprintPrFinding(finding); + return isPrFindingFingerprintDismissed(owner, repo, prNumber, fingerprint, headSha); +} + +function fingerprintPrFinding(finding: PrFinding): string { + return fingerprintPrFindingCommentBody(renderInlineFindingCommentBody(finding))!; +} + function renderFindingsSummary(findings: PrFinding[]): string { return findings .map((finding, index) => { @@ -117,7 +159,6 @@ async function replaceInlineFindingComments( headSha: string, findings: PrFinding[], ): Promise { - clearPrFindingDismissals(owner, repo, prNumber); await deleteInlineFindingComments(octokit, owner, repo, prNumber); const comments = findings @@ -167,6 +208,11 @@ async function deleteInlineFindingComments( } function renderInlineFindingComment(finding: PrFinding): string { + const body = renderInlineFindingCommentBody(finding); + return appendPrFindingFingerprint(body, fingerprintPrFindingCommentBody(body)!); +} + +function renderInlineFindingCommentBody(finding: PrFinding): string { const evidence = compactSentence(finding.short_evidence ?? finding.evidence, 180); const recommendation = compactSentence( finding.short_recommendation ?? finding.recommendation,