JSONbored · JSONbored · Jul 16, 2026 · Jul 16, 2026
@@ -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<string[]> {
+  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);

@@ -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<ReturnType<typeof buildPullRequestAdvisory>>;
+    repoFullName: string;
+    pullNumber: number;
+    screenshotTableGateConfig: ScreenshotTableGateConfig;
+    prBody: string | null | undefined;
+    prLabels: string[];
+    botCaptureSatisfied: boolean;
+    files: Awaited<ReturnType<typeof listPullRequestFiles>> | null;
+  },
+): Promise<void> {
+  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,13 +8518,24 @@ 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,
           additions: file.additions,
           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, {

@@ -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}