From 66350d7f4ee42650de8f6d4f85302770f0fde9be Mon Sep 17 00:00:00 2001 From: Ismail Pelaseyed Date: Mon, 25 May 2026 09:33:47 +0200 Subject: [PATCH 1/8] Point check runs and comments to superagent.sh. Set check run details_url to superagent.sh and remove the external contributor profile link from trust comments. --- src/lib/types.ts | 3 +- src/services/__tests__/checkRuns.test.ts | 42 ++++++++++++++++++++++++ src/services/__tests__/comments.test.ts | 15 +-------- src/services/checkRuns.ts | 3 ++ src/services/comments.ts | 10 ++---- 5 files changed, 51 insertions(+), 22 deletions(-) create mode 100644 src/services/__tests__/checkRuns.test.ts 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/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; } From 58ff2006979e24cfe0f05f40700f6f01913133a1 Mon Sep 17 00:00:00 2001 From: Ismail Pelaseyed Date: Mon, 25 May 2026 09:43:27 +0200 Subject: [PATCH 2/8] Persist finding dismissals across pushes. Track dismissed findings by stable fingerprint so accepted or resolved scanner comments are not recreated on later commits. --- src/lib/db.ts | 33 +++++++++++++--- src/services/__tests__/prScan.test.ts | 48 +++++++++++++++++++++++ src/services/findingDismissal.ts | 55 +++++++++++++++++++++++---- src/services/prFindingDismissals.ts | 20 +++++++++- src/services/prFindings.ts | 27 +++++++++++++ src/services/prScan.ts | 52 ++++++++++++++++++++++--- 6 files changed, 217 insertions(+), 18 deletions(-) diff --git a/src/lib/db.ts b/src/lib/db.ts index 486a018..0f8edc5 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,16 @@ 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 + LIMIT 1 + `), + clearPrFindingDismissals: db.prepare(` DELETE FROM pr_finding_dismissals WHERE owner = @owner AND repo = @repo AND pr_number = @prNumber diff --git a/src/services/__tests__/prScan.test.ts b/src/services/__tests__/prScan.test.ts index 8bfa0e9..26295be 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", () => ({ @@ -18,6 +21,7 @@ describe("runPrScan", () => { beforeEach(() => { vi.clearAllMocks(); vi.unstubAllGlobals(); + isPrFindingFingerprintDismissedMock.mockReturnValue(false); }); it("comments and flags the PR when local scanner returns findings", async () => { @@ -92,6 +96,50 @@ 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).toHaveBeenCalledWith( + expect.objectContaining({ comment_id: 100 }), + ); + }); + 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/findingDismissal.ts b/src/services/findingDismissal.ts index 7103c87..f459d9d 100644 --- a/src/services/findingDismissal.ts +++ b/src/services/findingDismissal.ts @@ -6,6 +6,7 @@ import { completeCheck } from "./checkRuns.js"; import { loadConfig } from "./config.js"; import { getReviewThreadIdForComment, + fingerprintPrFindingCommentBody, listPrFindingComments, listResolvedFindingCommentIds, resolveFindingForUserReply, @@ -95,6 +96,7 @@ export async function handleFindingReply( repo, prNumber, rootFindingId: rootFinding.id, + rootFindingBody: rootFinding.body, replyToCommentId: comment.id, dismissedBy: login || "unknown", acknowledgment: "Got it, thanks for the context.", @@ -109,6 +111,7 @@ export async function handleFindingReply( repo, prNumber, rootFindingId: rootFinding.id, + rootFindingBody: rootFinding.body, replyToCommentId: comment.id, dismissedBy: login || "unknown", acknowledgment: evaluation.acknowledgment, @@ -202,6 +205,7 @@ export async function handleFindingThreadResolved( repo, prNumber, reviewCommentId: rootFinding.id, + findingFingerprint: fingerprintPrFindingCommentBody(rootFinding.body), dismissedBy: login || "unknown", headSha: pr.head.sha, }); @@ -225,13 +229,22 @@ async function dismissFinding( repo: string; prNumber: number; rootFindingId: number; + rootFindingBody: string | null; replyToCommentId: number; dismissedBy: string; acknowledgment: string; }, ): Promise { - const { owner, repo, prNumber, rootFindingId, replyToCommentId, dismissedBy, acknowledgment } = - params; + const { + owner, + repo, + prNumber, + rootFindingId, + rootFindingBody, + replyToCommentId, + dismissedBy, + acknowledgment, + } = params; const { data: pr } = await octokit.rest.pulls.get({ owner, @@ -244,6 +257,7 @@ async function dismissFinding( repo, prNumber, reviewCommentId: rootFindingId, + findingFingerprint: fingerprintPrFindingCommentBody(rootFindingBody), dismissedBy, headSha: pr.head.sha, }); @@ -349,6 +363,34 @@ export async function reconcilePrScanAfterDismissals( } catch (err) { log.warn({ err }, "Failed to load resolved review threads"); } + + if (findings.length === 0) return; + + const headSha = + resolvedFindingIds.size > 0 + ? params.headSha ?? + ( + await octokit.rest.pulls.get({ + owner, + repo, + pull_number: prNumber, + }) + ).data.head.sha + : undefined; + + for (const finding of findings) { + if (!resolvedFindingIds.has(finding.id)) continue; + dismissPrFinding({ + owner, + repo, + prNumber, + reviewCommentId: finding.id, + findingFingerprint: fingerprintPrFindingCommentBody(finding.body), + dismissedBy: "resolved-thread", + headSha: headSha!, + }); + } + const openFindings = findings.filter( (finding) => !isPrFindingDismissed(owner, repo, prNumber, finding.id) && @@ -367,9 +409,8 @@ export async function reconcilePrScanAfterDismissals( return; } - if (findings.length === 0) return; - - const headSha = + const currentHeadSha = + headSha ?? params.headSha ?? ( await octokit.rest.pulls.get({ @@ -382,12 +423,12 @@ export async function reconcilePrScanAfterDismissals( 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; } diff --git a/src/services/prFindingDismissals.ts b/src/services/prFindingDismissals.ts index ee375aa..5f8437d 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,20 @@ export function isPrFindingDismissed( }); } +export function isPrFindingFingerprintDismissed( + owner: string, + repo: string, + prNumber: number, + findingFingerprint: string, +): boolean { + return !!queries.isPrFindingFingerprintDismissed.get({ + owner, + repo, + prNumber, + findingFingerprint, + }); +} + export function clearPrFindingDismissals( owner: string, repo: string, diff --git a/src/services/prFindings.ts b/src/services/prFindings.ts index 6fe49dc..23c5c75 100644 --- a/src/services/prFindings.ts +++ b/src/services/prFindings.ts @@ -1,4 +1,5 @@ import type { Octokit } from "octokit"; +import { createHash } from "node:crypto"; import { MARKERS } from "../lib/types.js"; export interface PrReviewComment { @@ -15,6 +16,32 @@ export function isPrFindingComment(body: string | null | undefined): boolean { return !!body?.includes(MARKERS.PR_FINDING) && !body.includes(MARKERS.PR_FINDING_ACK); } +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, diff --git a/src/services/prScan.ts b/src/services/prScan.ts index 650494a..086cf4c 100644 --- a/src/services/prScan.ts +++ b/src/services/prScan.ts @@ -7,10 +7,17 @@ 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 { + clearPrFindingDismissals, + isPrFindingFingerprintDismissed, +} from "./prFindingDismissals.js"; import { 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 +62,26 @@ export async function runPrScan( switch (status) { case "review": { + const findings = result.findings ?? []; + const openFindings = findings.filter( + (finding) => !isFindingDismissed(owner, repo, prNumber, 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); + await deleteInlineFindingComments(octokit, owner, repo, prNumber); + 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 +91,7 @@ export async function runPrScan( repo, prNumber, headSha, - result.findings ?? [], + openFindings, ); scheduleDismissalReconcile(octokit, { owner, repo, prNumber, headSha }); break; @@ -82,6 +105,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 +122,20 @@ export async function runPrScan( } } +function isFindingDismissed( + owner: string, + repo: string, + prNumber: number, + finding: PrFinding, +): boolean { + const fingerprint = fingerprintPrFinding(finding); + return isPrFindingFingerprintDismissed(owner, repo, prNumber, fingerprint); +} + +function fingerprintPrFinding(finding: PrFinding): string { + return fingerprintPrFindingCommentBody(renderInlineFindingCommentBody(finding))!; +} + function renderFindingsSummary(findings: PrFinding[]): string { return findings .map((finding, index) => { @@ -117,7 +155,6 @@ async function replaceInlineFindingComments( headSha: string, findings: PrFinding[], ): Promise { - clearPrFindingDismissals(owner, repo, prNumber); await deleteInlineFindingComments(octokit, owner, repo, prNumber); const comments = findings @@ -167,6 +204,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, From 1b90698611b405b7c15da0c6ace4c02f759f0df4 Mon Sep 17 00:00:00 2001 From: Ismail Pelaseyed Date: Mon, 25 May 2026 09:48:28 +0200 Subject: [PATCH 3/8] Scope finding dismissals to current head. Rehydrate reviewed findings for the active head before suppressing scanner comments so stale fingerprints do not suppress unrelated future findings. --- src/lib/db.ts | 1 + src/services/__tests__/prScan.test.ts | 14 ++- src/services/findingDismissal.ts | 123 +++++++++++++++----------- src/services/prFindingDismissals.ts | 2 + src/services/prFindings.ts | 22 +++++ src/services/prScan.ts | 12 ++- 6 files changed, 115 insertions(+), 59 deletions(-) diff --git a/src/lib/db.ts b/src/lib/db.ts index 0f8edc5..a9c4ef0 100644 --- a/src/lib/db.ts +++ b/src/lib/db.ts @@ -173,6 +173,7 @@ export const queries: Record = { AND repo = @repo AND pr_number = @prNumber AND finding_fingerprint = @findingFingerprint + AND head_sha = @headSha LIMIT 1 `), diff --git a/src/services/__tests__/prScan.test.ts b/src/services/__tests__/prScan.test.ts index 26295be..5e0c101 100644 --- a/src/services/__tests__/prScan.test.ts +++ b/src/services/__tests__/prScan.test.ts @@ -17,6 +17,11 @@ vi.mock("../../lib/logger.js", () => ({ }), })); +vi.mock("../findingDismissal.js", () => ({ + persistReviewedFindingDismissals: vi.fn().mockResolvedValue([]), + scheduleDismissalReconcile: vi.fn(), +})); + describe("runPrScan", () => { beforeEach(() => { vi.clearAllMocks(); @@ -135,8 +140,13 @@ describe("runPrScan", () => { expect.objectContaining({ labels: ["keep", "pr:verified"] }), ); expect(octokit.rest.pulls.createReview).not.toHaveBeenCalled(); - expect(octokit.rest.pulls.deleteReviewComment).toHaveBeenCalledWith( - expect.objectContaining({ comment_id: 100 }), + expect(octokit.rest.pulls.deleteReviewComment).not.toHaveBeenCalled(); + expect(isPrFindingFingerprintDismissedMock).toHaveBeenCalledWith( + "acme", + "repo", + 12, + expect.any(String), + "abc123", ); }); diff --git a/src/services/findingDismissal.ts b/src/services/findingDismissal.ts index f459d9d..a5b464b 100644 --- a/src/services/findingDismissal.ts +++ b/src/services/findingDismissal.ts @@ -7,6 +7,7 @@ import { loadConfig } from "./config.js"; import { getReviewThreadIdForComment, fingerprintPrFindingCommentBody, + listAcknowledgedFindingCommentIds, listPrFindingComments, listResolvedFindingCommentIds, resolveFindingForUserReply, @@ -350,58 +351,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 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 findings = await persistReviewedFindingDismissals(octokit, { + owner, + repo, + prNumber, + headSha: currentHeadSha, + }); if (findings.length === 0) return; - const headSha = - resolvedFindingIds.size > 0 - ? params.headSha ?? - ( - await octokit.rest.pulls.get({ - owner, - repo, - pull_number: prNumber, - }) - ).data.head.sha - : undefined; - - for (const finding of findings) { - if (!resolvedFindingIds.has(finding.id)) continue; - dismissPrFinding({ - owner, - repo, - prNumber, - reviewCommentId: finding.id, - findingFingerprint: fingerprintPrFindingCommentBody(finding.body), - dismissedBy: "resolved-thread", - headSha: headSha!, - }); - } - 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", @@ -409,17 +384,6 @@ export async function reconcilePrScanAfterDismissals( return; } - const currentHeadSha = - 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, @@ -443,6 +407,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, + }); + } + + 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 5f8437d..92d68c3 100644 --- a/src/services/prFindingDismissals.ts +++ b/src/services/prFindingDismissals.ts @@ -34,12 +34,14 @@ export function isPrFindingFingerprintDismissed( repo: string, prNumber: number, findingFingerprint: string, + headSha: string, ): boolean { return !!queries.isPrFindingFingerprintDismissed.get({ owner, repo, prNumber, findingFingerprint, + headSha, }); } diff --git a/src/services/prFindings.ts b/src/services/prFindings.ts index 23c5c75..6bc79b4 100644 --- a/src/services/prFindings.ts +++ b/src/services/prFindings.ts @@ -60,6 +60,28 @@ export async function listPrFindingComments( ); } +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 (comment.in_reply_to_id != null) ids.add(comment.in_reply_to_id); + } + + return ids; +} + export async function getReviewComment( octokit: Octokit, owner: string, diff --git a/src/services/prScan.ts b/src/services/prScan.ts index 086cf4c..bf50a94 100644 --- a/src/services/prScan.ts +++ b/src/services/prScan.ts @@ -11,7 +11,10 @@ import { clearPrFindingDismissals, isPrFindingFingerprintDismissed, } from "./prFindingDismissals.js"; -import { scheduleDismissalReconcile } from "./findingDismissal.js"; +import { + persistReviewedFindingDismissals, + scheduleDismissalReconcile, +} from "./findingDismissal.js"; import { clearLabels, ensureLabels, setLabel } from "./labels.js"; import { childLogger } from "../lib/logger.js"; import { @@ -62,9 +65,10 @@ 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, finding), + (finding) => !isFindingDismissed(owner, repo, prNumber, headSha, finding), ); if (!openFindings.length) { @@ -74,7 +78,6 @@ 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); break; } @@ -126,10 +129,11 @@ function isFindingDismissed( owner: string, repo: string, prNumber: number, + headSha: string, finding: PrFinding, ): boolean { const fingerprint = fingerprintPrFinding(finding); - return isPrFindingFingerprintDismissed(owner, repo, prNumber, fingerprint); + return isPrFindingFingerprintDismissed(owner, repo, prNumber, fingerprint, headSha); } function fingerprintPrFinding(finding: PrFinding): string { From a0fab33405ae08b3446ee5d04d69c0d1fa190235 Mon Sep 17 00:00:00 2001 From: Ismail Pelaseyed Date: Mon, 25 May 2026 09:58:27 +0200 Subject: [PATCH 4/8] Harden reviewed finding dismissal checks. Trust acknowledgment markers only from bot-authored replies and bind reviewed finding dismissals to the review comment commit SHA. --- .../__tests__/findingDismissal.test.ts | 53 +++++++++++++++++-- src/services/__tests__/prFindings.test.ts | 28 ++++++++++ src/services/findingDismissal.ts | 2 +- src/services/prFindings.ts | 6 +++ 4 files changed, 85 insertions(+), 4 deletions(-) diff --git a/src/services/__tests__/findingDismissal.test.ts b/src/services/__tests__/findingDismissal.test.ts index 6cdafbf..e7ca146 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"; @@ -265,13 +269,56 @@ 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", + }, + { + 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 ?? [ diff --git a/src/services/__tests__/prFindings.test.ts b/src/services/__tests__/prFindings.test.ts index 7403b46..a39ebd4 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"; @@ -48,6 +49,33 @@ describe("resolveRootFindingComment", () => { }); }); +describe("listAcknowledgedFindingCommentIds", () => { + it("only trusts bot-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: "superagent-security[bot]", type: "Bot" }, + }, + ]), + rest: { pulls: { listReviewComments } }, + } as any; + + const ids = await listAcknowledgedFindingCommentIds(octokit, "acme", "repo", 12); + + expect([...ids]).toEqual([20]); + }); +}); + describe("resolveFindingForUserReply", () => { it("matches a finding on the same file line when the reply has no in_reply_to_id", async () => { const octokit = { diff --git a/src/services/findingDismissal.ts b/src/services/findingDismissal.ts index a5b464b..b4c3da8 100644 --- a/src/services/findingDismissal.ts +++ b/src/services/findingDismissal.ts @@ -453,7 +453,7 @@ export async function persistReviewedFindingDismissals( reviewCommentId: finding.id, findingFingerprint: fingerprintPrFindingCommentBody(finding.body), dismissedBy: "reviewed-thread", - headSha, + headSha: finding.commit_id ?? headSha, }); } diff --git a/src/services/prFindings.ts b/src/services/prFindings.ts index 6bc79b4..f82af10 100644 --- a/src/services/prFindings.ts +++ b/src/services/prFindings.ts @@ -6,6 +6,7 @@ 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; @@ -76,12 +77,17 @@ export async function listAcknowledgedFindingCommentIds( const ids = new Set(); for (const comment of comments) { if (!comment.body?.includes(MARKERS.PR_FINDING_ACK)) continue; + if (!isBotUser(comment.user)) continue; if (comment.in_reply_to_id != null) ids.add(comment.in_reply_to_id); } return ids; } +function isBotUser(user: PrReviewComment["user"]): boolean { + return user?.type === "Bot" || !!user?.login?.endsWith("[bot]"); +} + export async function getReviewComment( octokit: Octokit, owner: string, From 352d49ccc245d4fda5d88e6a65bd093abd2ddbf5 Mon Sep 17 00:00:00 2001 From: Ismail Pelaseyed Date: Mon, 25 May 2026 10:05:31 +0200 Subject: [PATCH 5/8] Restrict finding acknowledgments to Superagent bot. Only trust hidden acknowledgment markers from the Superagent bot identity so other bots cannot suppress scanner findings. --- src/services/__tests__/prFindings.test.ts | 10 ++++++++-- src/services/prFindings.ts | 8 +++++--- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/services/__tests__/prFindings.test.ts b/src/services/__tests__/prFindings.test.ts index a39ebd4..12ef670 100644 --- a/src/services/__tests__/prFindings.test.ts +++ b/src/services/__tests__/prFindings.test.ts @@ -50,7 +50,7 @@ describe("resolveRootFindingComment", () => { }); describe("listAcknowledgedFindingCommentIds", () => { - it("only trusts bot-authored acknowledgment markers", async () => { + it("only trusts Superagent-authored acknowledgment markers", async () => { const listReviewComments = vi.fn(); const octokit = { paginate: vi.fn().mockResolvedValue([ @@ -64,6 +64,12 @@ describe("listAcknowledgedFindingCommentIds", () => { 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[bot]", type: "Bot" }, }, ]), @@ -72,7 +78,7 @@ describe("listAcknowledgedFindingCommentIds", () => { const ids = await listAcknowledgedFindingCommentIds(octokit, "acme", "repo", 12); - expect([...ids]).toEqual([20]); + expect([...ids]).toEqual([30]); }); }); diff --git a/src/services/prFindings.ts b/src/services/prFindings.ts index f82af10..15dee54 100644 --- a/src/services/prFindings.ts +++ b/src/services/prFindings.ts @@ -2,6 +2,8 @@ import type { Octokit } from "octokit"; import { createHash } from "node:crypto"; import { MARKERS } from "../lib/types.js"; +const SUPERAGENT_BOT_LOGIN = "superagent-security[bot]"; + export interface PrReviewComment { id: number; body: string | null; @@ -77,15 +79,15 @@ export async function listAcknowledgedFindingCommentIds( const ids = new Set(); for (const comment of comments) { if (!comment.body?.includes(MARKERS.PR_FINDING_ACK)) continue; - if (!isBotUser(comment.user)) continue; + if (!isSuperagentBot(comment.user)) continue; if (comment.in_reply_to_id != null) ids.add(comment.in_reply_to_id); } return ids; } -function isBotUser(user: PrReviewComment["user"]): boolean { - return user?.type === "Bot" || !!user?.login?.endsWith("[bot]"); +function isSuperagentBot(user: PrReviewComment["user"]): boolean { + return user?.login === SUPERAGENT_BOT_LOGIN && user.type === "Bot"; } export async function getReviewComment( From b062596473639092517459cf37de9c5ac2a8b95b Mon Sep 17 00:00:00 2001 From: Ismail Pelaseyed Date: Mon, 25 May 2026 10:12:22 +0200 Subject: [PATCH 6/8] Require Superagent author on finding comments. Ignore marker-bearing review comments unless they were authored by the Superagent bot before using them for dismissal matching. --- .../__tests__/findingDismissal.test.ts | 27 +++++++++++++++-- src/services/__tests__/prFindings.test.ts | 26 +++++++++++++++++ src/services/prFindings.ts | 29 +++++++++++++++---- 3 files changed, 74 insertions(+), 8 deletions(-) diff --git a/src/services/__tests__/findingDismissal.test.ts b/src/services/__tests__/findingDismissal.test.ts index e7ca146..e88a3fd 100644 --- a/src/services/__tests__/findingDismissal.test.ts +++ b/src/services/__tests__/findingDismissal.test.ts @@ -73,6 +73,7 @@ describe("handleFindingReply", () => { body: `${MARKERS.PR_FINDING}\n**P2:** Sandbox scopes`, path: "pkg.ts", line: 1, + user: { login: "superagent-security[bot]", type: "Bot" }, }, ], }); @@ -204,7 +205,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" }, + }, ], }); @@ -253,8 +260,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" }, + }, ], }); @@ -284,6 +303,7 @@ describe("persistReviewedFindingDismissals", () => { path: "a.ts", line: 1, commit_id: "comment-sha", + user: { login: "superagent-security[bot]", type: "Bot" }, }, { id: 11, @@ -327,6 +347,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 12ef670..0e90b07 100644 --- a/src/services/__tests__/prFindings.test.ts +++ b/src/services/__tests__/prFindings.test.ts @@ -33,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" }, }, }), }, @@ -91,6 +92,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() } }, @@ -105,4 +107,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/prFindings.ts b/src/services/prFindings.ts index 15dee54..930cf8b 100644 --- a/src/services/prFindings.ts +++ b/src/services/prFindings.ts @@ -19,6 +19,13 @@ 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; @@ -59,7 +66,7 @@ export async function listPrFindingComments( }); return comments.filter( - (comment) => isPrFindingComment(comment.body) && comment.path && comment.line, + (comment) => isSuperagentFindingComment(comment) && comment.path && comment.line, ); } @@ -87,7 +94,11 @@ export async function listAcknowledgedFindingCommentIds( } function isSuperagentBot(user: PrReviewComment["user"]): boolean { - return user?.login === SUPERAGENT_BOT_LOGIN && user.type === "Bot"; + return isSuperagentBotLogin(user?.login) && user?.type === "Bot"; +} + +function isSuperagentBotLogin(login: string | null | undefined): boolean { + return login === SUPERAGENT_BOT_LOGIN; } export async function getReviewComment( @@ -113,7 +124,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); } @@ -210,6 +221,7 @@ export async function listResolvedFindingCommentIds( nodes: Array<{ databaseId: number | null; body: string; + author?: { login?: string | null } | null; }>; }; }>; @@ -227,6 +239,9 @@ export async function listResolvedFindingCommentIds( nodes { databaseId body + author { + login + } } } } @@ -243,7 +258,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); } @@ -278,6 +293,7 @@ export async function resolveFindingForReviewThread( body: string; path?: string | null; line?: number | null; + author?: { login?: string | null } | null; }>; }; } | null; @@ -291,6 +307,9 @@ export async function resolveFindingForReviewThread( body path line + author { + login + } } } } @@ -300,7 +319,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 { From d5f5a012261a1a2bcb85fc932b2a2cf011bb2336 Mon Sep 17 00:00:00 2001 From: Ismail Pelaseyed Date: Mon, 25 May 2026 10:17:41 +0200 Subject: [PATCH 7/8] Bind direct finding dismissals to comment SHA. Use the root finding review comment commit when storing dismissal fingerprints so stale comments are not bound to the current PR head. --- src/services/__tests__/findingDismissal.test.ts | 7 +++++++ src/services/findingDismissal.ts | 6 +++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/services/__tests__/findingDismissal.test.ts b/src/services/__tests__/findingDismissal.test.ts index e88a3fd..3f74547 100644 --- a/src/services/__tests__/findingDismissal.test.ts +++ b/src/services/__tests__/findingDismissal.test.ts @@ -73,6 +73,7 @@ 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" }, }, ], @@ -116,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 () => { diff --git a/src/services/findingDismissal.ts b/src/services/findingDismissal.ts index b4c3da8..82d761f 100644 --- a/src/services/findingDismissal.ts +++ b/src/services/findingDismissal.ts @@ -98,6 +98,7 @@ export async function handleFindingReply( prNumber, rootFindingId: rootFinding.id, rootFindingBody: rootFinding.body, + rootFindingCommitId: rootFinding.commit_id, replyToCommentId: comment.id, dismissedBy: login || "unknown", acknowledgment: "Got it, thanks for the context.", @@ -113,6 +114,7 @@ export async function handleFindingReply( prNumber, rootFindingId: rootFinding.id, rootFindingBody: rootFinding.body, + rootFindingCommitId: rootFinding.commit_id, replyToCommentId: comment.id, dismissedBy: login || "unknown", acknowledgment: evaluation.acknowledgment, @@ -231,6 +233,7 @@ async function dismissFinding( prNumber: number; rootFindingId: number; rootFindingBody: string | null; + rootFindingCommitId?: string | null; replyToCommentId: number; dismissedBy: string; acknowledgment: string; @@ -242,6 +245,7 @@ async function dismissFinding( prNumber, rootFindingId, rootFindingBody, + rootFindingCommitId, replyToCommentId, dismissedBy, acknowledgment, @@ -260,7 +264,7 @@ async function dismissFinding( reviewCommentId: rootFindingId, findingFingerprint: fingerprintPrFindingCommentBody(rootFindingBody), dismissedBy, - headSha: pr.head.sha, + headSha: rootFindingCommitId ?? pr.head.sha, }); await postFindingReply(octokit, { From 0249de0fc1f64cb1148d02da902be59b4f007d9c Mon Sep 17 00:00:00 2001 From: Ismail Pelaseyed Date: Mon, 25 May 2026 10:27:04 +0200 Subject: [PATCH 8/8] Trust Superagent dev bot findings. Allow the local dev GitHub App identity when matching Superagent-authored finding and acknowledgment comments. --- src/services/__tests__/prFindings.test.ts | 8 +++++++- src/services/prFindings.ts | 9 +++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/services/__tests__/prFindings.test.ts b/src/services/__tests__/prFindings.test.ts index 0e90b07..d7d68ca 100644 --- a/src/services/__tests__/prFindings.test.ts +++ b/src/services/__tests__/prFindings.test.ts @@ -71,6 +71,12 @@ describe("listAcknowledgedFindingCommentIds", () => { 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" }, }, ]), @@ -79,7 +85,7 @@ describe("listAcknowledgedFindingCommentIds", () => { const ids = await listAcknowledgedFindingCommentIds(octokit, "acme", "repo", 12); - expect([...ids]).toEqual([30]); + expect([...ids]).toEqual([30, 40]); }); }); diff --git a/src/services/prFindings.ts b/src/services/prFindings.ts index 930cf8b..9f5fb93 100644 --- a/src/services/prFindings.ts +++ b/src/services/prFindings.ts @@ -2,7 +2,12 @@ import type { Octokit } from "octokit"; import { createHash } from "node:crypto"; import { MARKERS } from "../lib/types.js"; -const SUPERAGENT_BOT_LOGIN = "superagent-security[bot]"; +const SUPERAGENT_BOT_LOGINS = new Set([ + "superagent-security", + "superagent-security[bot]", + "superagent-security-dev", + "superagent-security-dev[bot]", +]); export interface PrReviewComment { id: number; @@ -98,7 +103,7 @@ function isSuperagentBot(user: PrReviewComment["user"]): boolean { } function isSuperagentBotLogin(login: string | null | undefined): boolean { - return login === SUPERAGENT_BOT_LOGIN; + return !!login && SUPERAGENT_BOT_LOGINS.has(login); } export async function getReviewComment(