diff --git a/src/github/pr-actions.ts b/src/github/pr-actions.ts index 43e7d4802f..7917733727 100644 --- a/src/github/pr-actions.ts +++ b/src/github/pr-actions.ts @@ -10,6 +10,10 @@ const ISSUE_EVENTS_RECENT_PAGE_LIMIT = 10; // earliest reviews on a PR with a long review history and could dismiss (or miss) the wrong one. const REVIEW_PAGE_SIZE = 100; const REVIEW_PAGE_LIMIT = 10; +// buildLowQualityCommitMessageFinding only inspects `commitMessages[0]` (the PR's oldest/primary commit, +// which is what GitHub's commits-list endpoint returns first — oldest-first, no sort override) so a single +// page is enough to give that signal a correct read regardless of how many commits the PR carries. +const COMMIT_MESSAGES_PAGE_SIZE = 100; // The GitHub write primitives the maintainer auto-maintain layer (#778) uses to act on a PR's STATE — never // its source. Thin wrappers over the installation-scoped REST API, mirroring labels.ts / comments.ts. Each @@ -180,6 +184,31 @@ export async function updatePullRequestBranch( }); } +/** The PR's commit subject+body messages, oldest-first (GitHub's default order for this endpoint, no sort + * override) — feeds the live gate's slop-assessment `low_quality_commit_message` signal + * (`buildLowQualityCommitMessageFinding`, weight 15), which was previously always skipped in production + * because nothing fetched and threaded this through `buildSlopAssessment`. Best-effort: any fetch failure + * (network, auth, rate limit) returns `[]`, degrading to the pre-fix behavior (the signal stays silent) + * rather than failing the whole gate evaluation over a non-essential enrichment call. */ +export async function listPullRequestCommitMessages(env: Env, installationId: number, repoFullName: string, pullNumber: number): Promise { + try { + const { owner, repo } = splitRepo(repoFullName); + return await withInstallationTokenRetry(env, installationId, async (token) => { + const octokit = makeInstallationOctokit(env, token, "live", githubRateLimitAdmissionKeyForInstallation(installationId)); + const response = await octokit.request("GET /repos/{owner}/{repo}/pulls/{pull_number}/commits", { + owner, + repo, + pull_number: pullNumber, + per_page: COMMIT_MESSAGES_PAGE_SIZE, + }); + const commits = response.data as Array<{ commit?: { message?: string | null } | null }>; + return commits.flatMap((entry) => (entry.commit?.message ? [entry.commit.message] : [])); + }); + } catch { + return []; + } +} + /** Post a plain issue/PR comment (used for the templated close message before closing). */ export async function createIssueComment(env: Env, installationId: number, repoFullName: string, issueNumber: number, body: string): Promise<{ id: number; html_url?: string | undefined }> { const { owner, repo } = splitRepo(repoFullName); diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 63d516fff9..2ccfe435a7 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -590,7 +590,7 @@ import { } from "../review/visual/preview-url"; import { resolveHardGuardrailGlobs } from "../review/guardrail-config"; import { guardrailPathMatches, isGuardrailHit } from "../signals/change-guardrail"; -import { createIssueComment } from "../github/pr-actions"; +import { createIssueComment, listPullRequestCommitMessages } from "../github/pr-actions"; import { loadLinkedIssueHardRules, mergeLinkedIssueHardRuleWithPersistedViolation, @@ -599,7 +599,7 @@ import { } from "../review/linked-issue-hard-rules"; import { DEFAULT_UNLINKED_ISSUE_GUARDRAIL } from "../review/unlinked-issue-guardrail-config"; import { resolveUnlinkedIssueMatchDisposition } from "../review/unlinked-issue-guardrail"; -import { DEFAULT_SCREENSHOT_TABLE_GATE, evaluateScreenshotTableGate, extractTableRowImageUrls } from "../review/screenshot-table-gate"; +import { DEFAULT_SCREENSHOT_CONTRACT_MESSAGE, DEFAULT_SCREENSHOT_TABLE_GATE, evaluateScreenshotTableGate, extractTableRowImageUrls, type ScreenshotTableGateConfig } from "../review/screenshot-table-gate"; import { isSafeHttpUrl } from "../review/content-lane/safe-url"; import { buildScreenshotTableVisionFindings, @@ -2742,8 +2742,11 @@ async function runAgentMaintenancePlanAndExecute( // (markPullRequestVisualCaptureSatisfied, written earlier in this same webhook by maybePublishPrPublicSurface // -- see that function's beforeAfter block -- and re-read here on `pr`, which this caller already re-fetched // fresh from the DB). Off by default (settings.screenshotTableGate.enabled === false), so the pure evaluator - // below is effectively free for the common case. "close" is the only enforcement action this gate has (#4110 - // removed the dead request_changes/comment surface) -- the check below is the ONLY place that reads `.action`. + // below is effectively free for the common case. "close" is the only ENFORCEMENT action this gate has (#4110 + // removed the dead request_changes/comment surface) -- the ternary below is the only place that folds a + // violation into an actual close. "advisory" mode gets its own separate, non-blocking visibility via + // maybeAddScreenshotTableAdvisoryFinding (this file), which re-evaluates the SAME pure check later in the + // main gate pass and appends a finding instead of a close. /* v8 ignore next -- defensive: resolveRepositorySettings always populates screenshotTableGate (getRepositorySettings's DB defaults), so this fallback is unreachable in practice. */ const screenshotTableGateConfig = settings.screenshotTableGate ?? DEFAULT_SCREENSHOT_TABLE_GATE; const botCaptureSatisfied = Boolean(pr.headSha) && pr.visualCaptureSatisfiedSha === pr.headSha; @@ -6806,6 +6809,65 @@ export async function maybeAddLockfileTamperFinding( } } +/** + * Screenshot-table gate advisory visibility (#2006 follow-up). `action: "close"` already communicates via its + * own templated close comment (see planAgentMaintenanceActions/screenshotTableCloseMessage), so a violation + * there never needs a SEPARATE advisory finding -- this only ever fires for `action: "advisory"`, which + * previously had NO visible effect at all: the live gate's only other `evaluateScreenshotTableGate` call site + * (`runAgentMaintenancePlanAndExecute`) discards the result entirely once `action !== "close"`. Mirrors + * `maybeAddLockfileTamperFinding` immediately above: off/out-of-scope is free, a violation appends ONE + * warning-severity, non-blocking finding (unrecognized by `isConfiguredGateBlocker`, so it can never gate), + * and any evaluation error is swallowed so it can never destabilize the gate. + */ +export async function maybeAddScreenshotTableAdvisoryFinding( + env: Env, + args: { + advisory: Awaited>; + repoFullName: string; + pullNumber: number; + screenshotTableGateConfig: ScreenshotTableGateConfig; + prBody: string | null | undefined; + prLabels: string[]; + botCaptureSatisfied: boolean; + files: Awaited> | null; + }, +): Promise { + if (!args.screenshotTableGateConfig.enabled || args.screenshotTableGateConfig.action !== "advisory") return; + try { + const files = + args.files ?? + (await listPullRequestFiles(env, args.repoFullName, args.pullNumber)); + const result = evaluateScreenshotTableGate({ + config: args.screenshotTableGateConfig, + prBody: args.prBody, + prLabels: args.prLabels, + changedFiles: files.map((file) => file.path), + botCaptureSatisfied: args.botCaptureSatisfied, + }); + if (!result.violated) return; + const detail = result.reason ?? DEFAULT_SCREENSHOT_CONTRACT_MESSAGE; + args.advisory.findings.push({ + code: "screenshot_table_missing", + severity: "warning", + title: "Missing before/after screenshot table", + detail, + action: "Add a before/after screenshot table to the pull request description (advisory only — this does not block merge).", + publicText: detail, + }); + } catch (error) { + /* v8 ignore next -- fail-safe: an evaluation error never destabilizes the gate. */ + console.error( + JSON.stringify({ + level: "error", + event: "screenshot_table_advisory_scan_failed", + repository: args.repoFullName, + pullNumber: args.pullNumber, + error: errorMessage(error), + }), + ); + } +} + /** * Run the linked-issue satisfaction assessment for advisory purposes (#1961/#3906) — opt-in via * `linkedIssueSatisfactionGateMode != "off"`. Assesses only the PR's PRIMARY (first) linked issue: v1 chooses @@ -8456,6 +8518,16 @@ async function maybePublishPrPublicSurface( } if (shouldCollectSlopEvidence(settings)) { const slopFiles = gateFiles ?? []; + // #slop-commit-messages: the low_quality_commit_message signal (weight 15) needs the PR's own commit + // subject(s), which nothing on this path previously fetched -- buildLowQualityCommitMessageFinding + // guards on commitMessages being undefined/empty and so silently never fired on the live gate. + // Best-effort: listPullRequestCommitMessages fails safe to [] (same as never having fetched it). + const slopCommitMessages = await listPullRequestCommitMessages( + env, + installationId, + repoFullName, + pr.number, + ); const slop = buildSlopAssessment({ changedFiles: slopFiles.map((file) => ({ path: file.path, @@ -8463,6 +8535,7 @@ async function maybePublishPrPublicSurface( deletions: file.deletions, })), description: pr.body, + commitMessages: slopCommitMessages, // Reuse the collision report already built for this gate run so a duplicate-cluster PR is flagged (#563). // Duplicate-winner adjudication (#dup-winner): the winner is judged on its OWN merits, so it is NOT // penalized for the cluster. Flag-OFF ⇒ isDupWinner is false ⇒ byte-identical to today. @@ -9355,6 +9428,20 @@ async function maybePublishPrPublicSurface( files: await getReviewFiles(), }); + // Screenshot-table gate advisory visibility (#2006 follow-up): `action: "advisory"` previously had no + // visible effect at all (see maybeAddScreenshotTableAdvisoryFinding's own doc comment). No-op for `off`, + // out-of-scope, or `action: "close"` (which already communicates via its own close comment). + await maybeAddScreenshotTableAdvisoryFinding(env, { + advisory, + repoFullName, + pullNumber: pr.number, + screenshotTableGateConfig: settings.screenshotTableGate ?? DEFAULT_SCREENSHOT_TABLE_GATE, + prBody: pr.body, + prLabels: pr.labels, + botCaptureSatisfied: Boolean(pr.headSha) && pr.visualCaptureSatisfiedSha === pr.headSha, + files: await getReviewFiles(), + }); + // Unresolved GitHub review threads (for example external security scanner inline findings) are blocking // review facts. Fetch them before gate evaluation so the normal blocker path drives the check-run, comment, // and disposition consistently. Fail-open on GitHub/GraphQL errors: a transient thread-read failure should not @@ -12470,12 +12557,17 @@ const COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS = 10 * 60_000; /** * Per-command @loopover rate limit (#2560, anti-abuse): generalizes review-nag's audit-ledger counting - * pattern (`countRecentAuditEventsForActorAndTarget`) to EVERY `@loopover` Q&A command, not just - * review-request pings. Keyed by `(actor, command, targetKey)` — the command name is folded into targetKey so - * repeatedly invoking ONE command never counts against a DIFFERENT command's own limit. Independent of, and - * complementary to, `maybeThrottleReviewNagPing` above: that one stays scoped to the thread's OWN author and - * can close a PR; this covers ANY authorized actor invoking ANY command and only ever holds (declines with a - * notice), never closes. Off (`commandRateLimitPolicy: "off"`, the default) is a complete no-op. + * pattern to EVERY `@loopover` Q&A command, not just review-request pings. The full `targetKey` (still + * `repo#issueNumber#command`, used for the redelivery guard and the audit-trail record below) stays + * per-thread, but the BUDGET COUNT itself is repo-wide via `countRecentAuditEventsForActorInRepoWithTargetSuffix` + * (#rate-limit-cross-thread-carryover, the same cross-PR-carryover fix #4021 already applied to review-nag's + * own cooldown) — pinned to THIS command's own suffix so one command's budget never bleeds into another's, but + * no longer resettable by simply invoking the command on a fresh issue/PR (a bare per-target count would reset + * to 0 the moment `issueNumber` changes, letting an actor who exhausts the limit on thread A get a full new + * budget on thread B). Independent of, and complementary to, `maybeThrottleReviewNagPing` above: that one stays + * scoped to the thread's OWN author and can close a PR; this covers ANY authorized actor invoking ANY command + * and only ever holds (declines with a notice), never closes. Off (`commandRateLimitPolicy: "off"`, the + * default) is a complete no-op. */ async function maybeThrottleLoopOverCommand( env: Env, @@ -12529,7 +12621,16 @@ async function maybeThrottleLoopOverCommand( /* v8 ignore next -- resolveRepositorySettings always resolves a concrete positive integer; the undefined side is defensive against the field's optional TS type. */ const windowHours = args.settings.commandRateLimitWindowHours ?? 24; const sinceIso = new Date(Date.now() - windowHours * 60 * 60 * 1000).toISOString(); - const priorInvocations = await countRecentAuditEventsForActorAndTarget(env, args.commenter, COMMAND_RATE_LIMIT_EVENT_TYPE, targetKey, sinceIso); + // Repo-wide, not per-target (#rate-limit-cross-thread-carryover), but still pinned to THIS command's own + // suffix so independently-budgeted commands never bleed into each other's count. + const priorInvocations = await countRecentAuditEventsForActorInRepoWithTargetSuffix( + env, + args.commenter, + COMMAND_RATE_LIMIT_EVENT_TYPE, + args.repoFullName, + args.command, + sinceIso, + ); const invocationCount = priorInvocations + 1; // this invocation counts too // Always record the invocation first so the running count reflects reality even when the rest of this @@ -12575,6 +12676,9 @@ async function maybeThrottleLoopOverCommand( } const INTENT_ROUTING_RATE_LIMIT_EVENT_TYPE = "github_app.intent_routing_invocation"; +// Shared between the full targetKey (redelivery guard / audit-trail record) and the repo-wide count's suffix +// filter below, so a naming drift between the two can never silently under/over-count (#rate-limit-cross-thread-carryover). +const INTENT_ROUTING_TARGET_SUFFIX = "intent-routing"; /** * Dedicated rate limit for the intent-classification router (#4596): every unrecognized-verb mention with @@ -12601,7 +12705,7 @@ async function maybeThrottleIntentRouting( const policy = args.settings.commandRateLimitPolicy ?? "off"; if (policy === "off") return true; - const targetKey = `${args.repoFullName}#${args.issueNumber}#intent-routing`; + const targetKey = `${args.repoFullName}#${args.issueNumber}#${INTENT_ROUTING_TARGET_SUFFIX}`; const redeliverySinceIso = new Date(Date.now() - COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS).toISOString(); const alreadySeen = await hasAuditEventForDelivery(env, args.commenter, INTENT_ROUTING_RATE_LIMIT_EVENT_TYPE, targetKey, args.deliveryId, redeliverySinceIso); // A redelivered webhook must not re-classify (and re-spend shared neuron budget) for one real mention. @@ -12612,7 +12716,16 @@ async function maybeThrottleIntentRouting( /* v8 ignore next -- resolveRepositorySettings always resolves a concrete positive integer; the undefined side is defensive against the field's optional TS type. */ const windowHours = args.settings.commandRateLimitWindowHours ?? 24; const sinceIso = new Date(Date.now() - windowHours * 60 * 60 * 1000).toISOString(); - const priorInvocations = await countRecentAuditEventsForActorAndTarget(env, args.commenter, INTENT_ROUTING_RATE_LIMIT_EVENT_TYPE, targetKey, sinceIso); + // Repo-wide, not per-target (#rate-limit-cross-thread-carryover) -- same fix as maybeThrottleLoopOverCommand + // above, mirroring review-nag's #4021 cross-PR-carryover pattern. + const priorInvocations = await countRecentAuditEventsForActorInRepoWithTargetSuffix( + env, + args.commenter, + INTENT_ROUTING_RATE_LIMIT_EVENT_TYPE, + args.repoFullName, + INTENT_ROUTING_TARGET_SUFFIX, + sinceIso, + ); const invocationCount = priorInvocations + 1; await recordAuditEvent(env, { diff --git a/src/types.ts b/src/types.ts index 4bce741f31..d7c3240720 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1283,8 +1283,10 @@ export type RepositorySettings = { * `"advisory"` (#4535) is a NEW, actually-wired value, not a resurrection of either removed one: the gate * still computes the violation and its reason, but `src/queue/processors.ts` only ever folds the result into * the close-triggering `screenshotTableMatch` when `action === "close"` -- so `"advisory"` is a real no-op on - * merge/close by construction, with visibility left to the AI reviewer's own commentary (its context is - * expected to mention the same completeness requirement -- see the review-context sync in the #4540 PR). */ + * merge/close by construction. Visibility comes from `maybeAddScreenshotTableAdvisoryFinding` (queue/processors.ts), + * which appends a non-blocking `screenshot_table_missing` finding to the PR's advisory panel whenever + * `action === "advisory"` and the gate would have violated -- a deterministic signal, not just left to chance + * in the AI reviewer's own commentary. */ export type ScreenshotTableGateAction = "close" | "advisory"; /** Per-repo config for the before/after screenshot-table gate (#2006). See {@link RepositorySettings.screenshotTableGate} diff --git a/test/unit/github-pr-actions.test.ts b/test/unit/github-pr-actions.test.ts index 20c7a1bb47..3e3293c145 100644 --- a/test/unit/github-pr-actions.test.ts +++ b/test/unit/github-pr-actions.test.ts @@ -1,6 +1,6 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { generateKeyPairSync } from "node:crypto"; -import { closeIssue, closePullRequest, createIssueComment, createPullRequestReview, createPullRequestReviewComments, dismissLatestBotApproval, getLastCloserLogin, getLastReopenerLogin, mergePullRequest, reopenPullRequest, updatePullRequestBranch } from "../../src/github/pr-actions"; +import { closeIssue, closePullRequest, createIssueComment, createPullRequestReview, createPullRequestReviewComments, dismissLatestBotApproval, getLastCloserLogin, getLastReopenerLogin, listPullRequestCommitMessages, mergePullRequest, reopenPullRequest, updatePullRequestBranch } from "../../src/github/pr-actions"; import { clearInstallationTokenCacheForTest } from "../../src/github/app"; import { createTestEnv } from "../helpers/d1"; @@ -234,6 +234,45 @@ describe("GitHub PR action primitives (#778)", () => { expect(result).toEqual({ id: 9, html_url: "https://github.com/owner/repo/pull/7#issuecomment-9" }); }); + it("#slop-commit-messages: fetches the PR's commit subject lines, oldest-first, from the pulls/commits endpoint", async () => { + const calls: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + calls.push(url); + return Response.json([ + { sha: "c1", commit: { message: "wip" } }, + { sha: "c2", commit: { message: "feat(api): add cursor pagination" } }, + ]); + }); + const result = await listPullRequestCommitMessages(envWithKey(), 123, "owner/repo", 7); + expect(result).toEqual(["wip", "feat(api): add cursor pagination"]); + expect(calls[0]).toMatch(/\/repos\/owner\/repo\/pulls\/7\/commits/); + }); + + it("#slop-commit-messages: drops commit entries with no message rather than inserting an empty string", async () => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + return Response.json([{ sha: "c1", commit: { message: "" } }, { sha: "c2", commit: null }, { sha: "c3" }, { sha: "c4", commit: { message: "fix: real subject" } }]); + }); + const result = await listPullRequestCommitMessages(envWithKey(), 123, "owner/repo", 7); + expect(result).toEqual(["fix: real subject"]); + }); + + it("#slop-commit-messages: fails safe to an empty array on a GitHub error, never throws (so the slop gate degrades to pre-fix behavior, not a hard failure)", async () => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + return new Response("server error", { status: 500 }); + }); + await expect(listPullRequestCommitMessages(envWithKey(), 123, "owner/repo", 7)).resolves.toEqual([]); + }); + + it("#slop-commit-messages: fails safe to an empty array on an invalid repo name, never throws", async () => { + await expect(listPullRequestCommitMessages(createTestEnv(), 1, "invalid", 4)).resolves.toEqual([]); + }); + it("walks paginated issue events to find the true most recent closer", async () => { const calls: string[] = []; vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index f4fdf80611..dfb573c15d 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -1401,6 +1401,71 @@ describe("queue processors", () => { expect(seen.comments.some((c) => c.includes("before/after screenshot table"))).toBe(true); }); + it("REGRESSION (#2006 advisory follow-up): action: \"advisory\" is no longer a silent no-op -- a missing before/after table appends a visible, non-blocking finding instead of closing the PR", async () => { + // Before the fix, evaluateScreenshotTableGate's `violated` result was only ever folded into a real signal + // when `action === "close"` -- the "advisory" ternary branch discarded it entirely, so a maintainer who + // deliberately chose the softer "advisory" action got neither enforcement NOR visibility. No `autonomy` + // is configured here (unlike the "close" test above) -- advisory mode's finding comes from the main gate + // pass, not the agent-maintenance close path, so it must appear even with the agent fully unconfigured. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["pull_request"] }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "all_prs", + publicSurface: "comment_only", + checkRunMode: "off", + reviewCheckMode: "required", + }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { screenshotTableGate: { enabled: true, whenLabels: ["visual"], action: "advisory" } } }, "repo_file"); + const seen = { closed: false, comments: [] as string[] }; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/58/files")) return Response.json([{ filename: "apps/ui/src/App.tsx", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+const ok = true;" }]); + if (url.includes("/pulls/58/reviews")) return Response.json([]); + if (url.includes("/pulls/58/commits")) return Response.json([]); + if (url.endsWith("/pulls/58") && method === "PATCH") { seen.closed = JSON.parse(String(init?.body ?? "{}")).state === "closed"; return Response.json({ number: 58, state: "closed" }); } + if (url.endsWith("/pulls/58")) return Response.json({ number: 58, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis58" }, mergeable_state: "clean" }); + if (url.includes("/commits/vis58/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/vis58/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/58/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/58/comments") && method === "POST") { + seen.comments.push(String(JSON.parse(String(init?.body ?? "{}")).body ?? "")); + return Response.json({ id: 1 }, { status: 201 }); + } + if (url.includes("/issues/58/comments")) return Response.json([]); + return Response.json({}); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "screenshot-table-advisory", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 58, title: "New button color", state: "open", user: { login: "visual-contributor" }, head: { sha: "vis58" }, labels: [{ name: "visual" }], body: "Changed the button color.", mergeable_state: "clean", reviewDecision: "APPROVED" }, + }, + }); + + // Never closed -- "advisory" must not enforce, only surface a finding. + expect(seen.closed).toBe(false); + const closeAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'agent.action.close'").first<{ n: number }>(); + expect(closeAudit?.n ?? 0).toBe(0); + // The finding surfaces in the published unified PR comment's advisory findings section (the `advisories` + // DB table row is written once, EARLY, before this late-appended finding exists -- so the live-published + // comment, not that row, is the correct place to observe it). + const finalComment = seen.comments.at(-1) ?? ""; + expect(finalComment).toContain("Missing before/after screenshot table"); + expect(finalComment).toContain("before/after screenshot table to the pull request description"); + }); + it("screenshot-table gate (#2006): an in-scope PR WITH a valid before/after table is NOT closed by the gate (no false-positive)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), diff --git a/test/unit/queue-5.test.ts b/test/unit/queue-5.test.ts index 3dc2482867..eb7c580115 100644 --- a/test/unit/queue-5.test.ts +++ b/test/unit/queue-5.test.ts @@ -1234,6 +1234,27 @@ describe("queue processors", () => { expect(applied?.n).toBe(0); }); + it("REGRESSION: the budget is repo-wide, not per-thread — opening a fresh issue/PR does not reset a command's rate-limit counter", async () => { + // Before the fix, the count query was scoped to the EXACT targetKey (repo#issueNumber#command), so an + // actor who exhausted the limit on one thread could get a full new budget simply by invoking the SAME + // command on a DIFFERENT issue/PR number — the exact bypass class #4021 already fixed for review-nag. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", commandRateLimitPolicy: "hold", commandRateLimitMaxPerWindow: 1, commandRateLimitWindowHours: 24 }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 340, title: "Rate limit target A", state: "open", user: { login: "oktofeesh1" }, author_association: "NONE", labels: [], body: "" }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 341, title: "Rate limit target B (fresh)", state: "open", user: { login: "oktofeesh1" }, author_association: "NONE", labels: [], body: "" }); + // Already at the "help" limit (1) on issue #340. + await repositoriesModule.recordAuditEvent(env, { eventType: "github_app.command_invocation", actor: "maintainer", targetKey: "JSONbored/gittensory#340#help", outcome: "completed" }); + const seen = { comments: [] as string[] }; + stubCommandRateLimitFetch(341, seen); + // The SAME command ("help"), by the SAME actor, on a BRAND-NEW issue #341 — must still be held. + await processJob(env, { type: "github-webhook", deliveryId: "rl-cross-thread-carryover", eventName: "issue_comment", payload: mentionPayload(341, "@loopover help") }); + expect(seen.comments).toHaveLength(1); + expect(seen.comments[0]).toContain("rate limit"); + const applied = await env.DB.prepare("select outcome, detail from audit_events where event_type = 'github_app.command_rate_limit_applied'").first<{ outcome: string; detail: string }>(); + expect(applied?.outcome).toBe("completed"); + expect(applied?.detail).toContain("hold applied"); + }); + it("dry-run mode: holds the command but never posts a live cooldown comment", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", commandRateLimitPolicy: "hold", commandRateLimitMaxPerWindow: 1, commandRateLimitWindowHours: 24, agentDryRun: true }); @@ -1814,6 +1835,45 @@ describe("queue processors", () => { expect(seen.comments[0]).not.toContain("LoopOver readiness blockers"); }); + it("REGRESSION: the intent-routing budget is repo-wide, not per-thread — opening a fresh issue/PR does not reset the classifier ceiling", async () => { + // Same cross-thread-carryover bug as maybeThrottleLoopOverCommand above, applied to the intent-routing + // classifier's own independent counter. + const advisoryRun = vi.fn(async () => ({ response: '{"command": "blockers"}' })); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), AI_ADVISORY: { run: advisoryRun } as unknown as Ai }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", commandRateLimitPolicy: "hold", commandRateLimitAiMaxPerWindow: 1, commandRateLimitWindowHours: 24 }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 342, title: "Rate limit target A", state: "open", user: { login: "oktofeesh1" }, author_association: "NONE", labels: [], body: "" }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 343, title: "Rate limit target B (fresh)", state: "open", user: { login: "oktofeesh1" }, author_association: "NONE", labels: [], body: "" }); + // Already at the ceiling (1) on issue #342. + await repositoriesModule.recordAuditEvent(env, { + eventType: "github_app.intent_routing_invocation", + actor: "maintainer", + targetKey: "JSONbored/gittensory#342#intent-routing", + outcome: "completed", + }); + const seen = { comments: [] as string[] }; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("raw.githubusercontent.com") && url.includes(".loopover.yml")) { + return new Response("settings:\n advisoryAiRouting:\n intentRouting: true\n", { status: 200 }); + } + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes("/collaborators/") && url.includes("/permission")) return Response.json({ permission: "maintain" }); + if (url.includes("/issues/343/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/343/comments") && method === "POST") { + seen.comments.push(String(JSON.parse(String(init?.body ?? "{}")).body ?? "")); + return Response.json({ id: seen.comments.length }, { status: 201 }); + } + return new Response("not found", { status: 404 }); + }); + // A BRAND-NEW issue #343 (same actor, same repo) must still be held -- the classifier never runs. + await processJob(env, { type: "github-webhook", deliveryId: "intent-routing-cross-thread-carryover", eventName: "issue_comment", payload: mentionPayload(343, "@loopover why is this stuck?") }); + expect(advisoryRun).not.toHaveBeenCalled(); + expect(seen.comments).toHaveLength(1); // fails open: the plain did-you-mean fallback still posts + expect(seen.comments[0]).not.toContain("Interpreted"); + expect(seen.comments[0]).not.toContain("LoopOver readiness blockers"); + }); + it("#4596: REGRESSION: a redelivered webhook does not re-classify or double-count the intent-routing invocation", async () => { const advisoryRun = vi.fn(async () => ({ response: '{"command": "blockers"}' })); const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), AI_ADVISORY: { run: advisoryRun } as unknown as Ai }); @@ -2564,6 +2624,94 @@ describe("queue processors", () => { expect(refreshedFiles).toHaveLength(1); }); + it("REGRESSION (#slop-commit-messages): the live slop gate fetches the PR's own commit messages, so low_quality_commit_message can actually fire", async () => { + // Before the fix, buildSlopAssessment was called with no `commitMessages` at all on this path -- the + // detector guards on `commitMessages === undefined` and so unconditionally returned null in production, + // no matter how generic the PR's real commit subject was. Two PRs with distinct titles/bodies (so the + // collision report's own title/term-overlap check never clusters them as duplicates of each other) each + // pairing a source file with a substantive test file (so missingTestEvidence never fires either) -- + // differing ONLY in their (stubbed) GitHub commit history, isolating the commit-message signal from every + // other slop detector. Asserted via the persisted `pull_requests` slopRisk/slopBand columns + // (updatePullRequestSlopAssessment runs unconditionally inside shouldCollectSlopEvidence, independent of + // the publish/comment pipeline), mirroring how "clears the persisted dashboard slop score when the slop + // gate is off (#911)" above verifies the same live score. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload({ "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, { kind: "raw-github", url: "https://example.test" }, "2026-05-23T00:00:00.000Z"), + ); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "off", + publicSurface: "off", + autoLabelEnabled: false, + checkRunMode: "off", + reviewCheckMode: "required", + linkedIssueGateMode: "off", + slopGateMode: "advisory", + }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 96, title: "Add clamp helper for the volume slider", state: "open", user: { login: "contributor" }, author_association: "CONTRIBUTOR", head: { sha: "genericsha96" }, labels: [], body: "Bounds the volume slider's raw input to a safe numeric range." }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 97, title: "Restrict pagination cursor offsets to a valid window", state: "open", user: { login: "contributor" }, author_association: "CONTRIBUTOR", head: { sha: "specificsha97" }, labels: [], body: "Prevents the pagination cursor from resolving to an out-of-range offset." }); + + const changedFiles = [ + { filename: "src/clamp.ts", status: "added", additions: 8, deletions: 0, changes: 8 }, + { filename: "src/clamp.test.ts", status: "added", additions: 5, deletions: 0, changes: 5 }, + ]; + let checkRunSeq = 960; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/96/files")) return Response.json(changedFiles); + if (url.includes("/pulls/97/files")) return Response.json(changedFiles); + // PR #96's ONLY commit has a generic, empty-of-detail subject -- must trip low_quality_commit_message. + if (url.includes("/pulls/96/commits")) return Response.json([{ sha: "c1", commit: { message: "wip" } }]); + // PR #97's commit is a real, specific Conventional Commit -- must NOT trip the same finding. + if (url.includes("/pulls/97/commits")) return Response.json([{ sha: "c2", commit: { message: "feat(clamp): add clamp(value, min, max) helper" } }]); + if (url.includes("/commits/genericsha96/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/specificsha97/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.endsWith("/pulls/96")) return Response.json({ number: 96, state: "open", user: { login: "contributor" }, head: { sha: "genericsha96" }, mergeable_state: "clean" }); + if (url.endsWith("/pulls/97")) return Response.json({ number: 97, state: "open", user: { login: "contributor" }, head: { sha: "specificsha97" }, mergeable_state: "clean" }); + if (url.includes("/check-runs") && init?.method === "POST") { checkRunSeq += 1; return Response.json({ id: checkRunSeq }, { status: 201 }); } + if (url.includes("/check-runs")) return Response.json({ id: checkRunSeq }); + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "slop-commit-messages-generic", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 96, title: "Add clamp helper for the volume slider", state: "open", user: { login: "contributor" }, author_association: "CONTRIBUTOR", head: { sha: "genericsha96" }, labels: [], body: "Bounds the volume slider's raw input to a safe numeric range." }, + }, + }); + await processJob(env, { + type: "github-webhook", + deliveryId: "slop-commit-messages-specific", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 97, title: "Restrict pagination cursor offsets to a valid window", state: "open", user: { login: "contributor" }, author_association: "CONTRIBUTOR", head: { sha: "specificsha97" }, labels: [], body: "Prevents the pagination cursor from resolving to an out-of-range offset." }, + }, + }); + + const generic = await getPullRequest(env, "JSONbored/gittensory", 96); + const specific = await getPullRequest(env, "JSONbored/gittensory", 97); + // "wip" alone trips ONLY low_quality_commit_message (weight 15) -- clean of every other detector by + // construction (a real test file, a non-empty description, no duplicate cluster). + expect(generic?.slopRisk).toBe(15); + expect(generic?.slopBand).toBe("low"); + // The specific Conventional Commit trips nothing at all. + expect(specific?.slopRisk).toBe(0); + expect(specific?.slopBand).toBe("clean"); + }); + it("#dup-winner: flag ON spares the lowest open sibling — no duplicate block, slop not penalized for the cluster", async () => { // LOOPOVER_DUPLICATE_WINNER ON. A same-issue cluster of OPEN PRs (#91 winner, #92 loser) under // duplicatePrGateMode: block. The winner (#91, lowest open number) must NOT be gate-blocked or slop-