fix(review): include violation count and ban proximity in moderation escalation comments

src/services/agent-action-executor.ts

@@ -36,6 +36,7 @@ import {
   resolveEffectiveModerationRules,
   resolveModerationGateEnabled,
   type ModerationRuleType,
+  type ModerationTier,
 } from "../settings/moderation-rules";
 import { incr } from "../selfhost/metrics";
 import { shouldWaitForOlderSiblings } from "../review/merge-train";
@@ -604,6 +605,33 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE
 
 const MODERATION_RULE_TYPES = new Set<string>(Object.keys(MODERATION_VIOLATION_EVENT_TYPE));
 
+/** Pure text for the moderation-escalation follow-up comment (#mod-warning-context): the warning/banned LABEL
+ *  alone doesn't tell a contributor (or a maintainer reading the closure later) how many violations are on
+ *  record or how close they are to an automatic ban -- this always accompanies the label with the actual
+ *  numbers. Exported for direct unit testing without driving the full escalation flow. `totalCount < banThreshold`
+ *  is guaranteed by the `tier === "warning"` caller contract (moderationTierForViolationCount only returns
+ *  "warning" below the threshold), so the remaining-count subtraction is never non-positive here. */
+export function buildModerationEscalationComment(args: {
+  tier: Exclude<ModerationTier, "none">;
+  totalCount: number;
+  banThreshold: number;
+  violationDecayDays: number | null;
+  blacklisted: boolean;
+}): string {
+  const decayNote =
+    args.violationDecayDays !== null
+      ? ` Violations older than ${args.violationDecayDays} day(s) no longer count toward this total.`
+      : "";
+  if (args.tier === "banned") {
+    const blacklistNote = args.blacklisted
+      ? " This contributor has been automatically added to the blacklist."
+      : " Automatic blacklisting is not enabled for this instance, so no blacklist entry was added.";
+    return `This contributor now has ${args.totalCount} recorded moderation violation(s), at or beyond the configured threshold of ${args.banThreshold}.${blacklistNote}${decayNote}`;
+  }
+  const remaining = args.banThreshold - args.totalCount;
+  return `This contributor now has ${args.totalCount} recorded moderation violation(s) (warning threshold). ${remaining} more will result in an automatic ban.${decayNote}`;
+}
+
 /**
  * Moderation-rules engine (#selfhost-mod-engine / #review-evasion-protection): given that a moderation-
  * tracked enforcement action for `rule` ALREADY COMPLETED against `authorLogin` on `repoFullName#number`,
@@ -652,16 +680,38 @@ export async function applyModerationEscalationForRule(
   const label = tier === "banned" ? (args.moderationSettings?.moderationBannedLabel ?? globalConfig.bannedLabel) : (args.moderationSettings?.moderationWarningLabel ?? globalConfig.warningLabel);
   await ensurePullRequestLabel(env, args.installationId, args.repoFullName, args.number, label, { createMissingLabel: true }).catch(() => undefined);
 
+  let blacklisted = false;
   if (tier === "banned" && globalConfig.autoBlacklistOnBan) {
     /* v8 ignore next -- getGlobalContributorBlacklist never actually resolves undefined (it fails open to
        `[]`); the `?? []` only satisfies RepositorySettings["contributorBlacklist"]'s optional TS type. */
     const current = (await getGlobalContributorBlacklist(env)) ?? [];
-    if (!isAuthorBlacklisted(args.authorLogin, current)) {
+    if (isAuthorBlacklisted(args.authorLogin, current)) {
+      blacklisted = true;
+    } else {
       const banReason = `moderation-engine auto-ban: ${totalCount} lifetime violations reached the configured threshold`;
       const nextBlacklist = [...current, { login: args.authorLogin, reason: banReason, evidence: [targetKey] }];
-      await upsertGlobalContributorBlacklist(env, { contributorBlacklist: nextBlacklist }).catch(() => undefined);
+      // A write failure here must not throw (this whole function is best-effort, matching the label
+      // application above) -- it degrades `blacklisted` to false so the escalation comment below correctly
+      // says no blacklist entry was added, rather than claiming a ban that didn't actually happen.
+      blacklisted = await upsertGlobalContributorBlacklist(env, { contributorBlacklist: nextBlacklist })
+        .then(() => true)
+        .catch(() => false);
     }
   }
+
+  // #mod-warning-context: a maintainer (or the contributor) reading the closure later has no way to know how
+  // many violations are on record or how close this contributor is to an automatic ban from the label alone --
+  // post it as a plain follow-up comment (best-effort, matching every other side effect in this function)
+  // rather than threading it back into the original close comment, which is already posted by the time
+  // escalation runs.
+  const escalationComment = buildModerationEscalationComment({
+    tier,
+    totalCount,
+    banThreshold: globalConfig.banThreshold,
+    violationDecayDays: globalConfig.violationDecayDays,
+    blacklisted,
+  });
+  await createIssueComment(env, args.installationId, args.repoFullName, args.number, escalationComment).catch(() => undefined);
 }
 
 /**

test/unit/agent-action-executor.test.ts

@@ -59,6 +59,7 @@ import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequ
 import {
   actionParams,
   applyModerationEscalationForRule,
+  buildModerationEscalationComment,
   clearInstallationHealthRefreshCooldownForTest,
   clearWritePermissionDenialCooldownForTest,
   writePermissionDenialCooldownSizeForTest,
@@ -1519,6 +1520,14 @@ describe("moderation-rules engine escalation (#selfhost-mod-engine)", () => {
     expect(ensurePullRequestLabel).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "mod:warning", { createMissingLabel: true });
   });
 
+  it("#mod-warning-context: the label alone is not enough -- a follow-up comment states the violation count and how many remain before an automatic ban", async () => {
+    const env = createTestEnv({});
+    await upsertGlobalModerationConfig(env, { enabled: true, banThreshold: 5 });
+    await executeAgentMaintenanceActions(env, ctx({ authorLogin: "farmer99" }), [coupledClose, coupledLabel]);
+    expect(createIssueComment).toHaveBeenCalledWith(env, 123, "owner/repo", 7, expect.stringContaining("1 recorded moderation violation"));
+    expect(createIssueComment).toHaveBeenCalledWith(env, 123, "owner/repo", 7, expect.stringContaining("4 more will result in an automatic ban"));
+  });
+
   it("4 violations -> warning only; the 5th (default threshold) escalates to mod:banned + auto-blacklists the login", async () => {
     const env = createTestEnv({});
     await upsertGlobalModerationConfig(env, { enabled: true });
@@ -1535,26 +1544,30 @@ describe("moderation-rules engine escalation (#selfhost-mod-engine)", () => {
     expect(ensurePullRequestLabel).toHaveBeenCalledWith(env, 123, "owner/repo", 104, "mod:banned", { createMissingLabel: true });
     const blacklist = await getGlobalContributorBlacklist(env);
     expect(blacklist?.map((entry) => entry.login)).toContain("farmer99");
+    expect(createIssueComment).toHaveBeenCalledWith(env, 123, "owner/repo", 104, expect.stringContaining("automatically added to the blacklist"));
   });
 
-  it("does NOT auto-blacklist when autoBlacklistOnBan is off, even at the ban threshold", async () => {
+  it("does NOT auto-blacklist when autoBlacklistOnBan is off, even at the ban threshold -- the follow-up comment says so explicitly", async () => {
     const env = createTestEnv({});
     await upsertGlobalModerationConfig(env, { enabled: true, banThreshold: 1, autoBlacklistOnBan: false });
     await executeAgentMaintenanceActions(env, ctx({ authorLogin: "farmer99" }), [coupledClose, coupledLabel]);
     expect(ensurePullRequestLabel).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "mod:banned", { createMissingLabel: true });
     const blacklist = await getGlobalContributorBlacklist(env);
     expect(blacklist?.map((entry) => entry.login)).not.toContain("farmer99");
+    expect(createIssueComment).toHaveBeenCalledWith(env, 123, "owner/repo", 7, expect.stringContaining("Automatic blacklisting is not enabled"));
   });
 
-  it("does not double-add an actor who is already on the global blacklist", async () => {
+  it("does not double-add an actor who is already on the global blacklist -- the follow-up comment still reports the (already-true) blacklisted state", async () => {
     const env = createTestEnv({});
     await upsertGlobalModerationConfig(env, { enabled: true, banThreshold: 1 });
     // Two DISTINCT PRs (different pullNumber) -- two genuinely separate violations, not a same-target replay
     // (which the idempotency fix would correctly no-op before ever reaching the blacklist-membership check).
     await executeAgentMaintenanceActions(env, ctx({ authorLogin: "farmer99", pullNumber: 7 }), [coupledClose, coupledLabel]);
+    vi.clearAllMocks();
     await executeAgentMaintenanceActions(env, ctx({ authorLogin: "farmer99", pullNumber: 8 }), [{ ...coupledClose }, { ...coupledLabel }]);
     const blacklist = await getGlobalContributorBlacklist(env);
     expect(blacklist?.filter((entry) => entry.login === "farmer99")).toHaveLength(1);
+    expect(createIssueComment).toHaveBeenCalledWith(env, 123, "owner/repo", 8, expect.stringContaining("automatically added to the blacklist"));
   });
 
   it("per-repo moderationGateMode 'off' force-disables the layer even when the global config is enabled", async () => {
@@ -1650,6 +1663,7 @@ describe("moderation-rules engine escalation (#selfhost-mod-engine)", () => {
     // Only the JUST-recorded violation counts (1 < banThreshold 2) -> warning, not banned.
     expect(ensurePullRequestLabel).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "mod:warning", { createMissingLabel: true });
     expect(ensurePullRequestLabel).not.toHaveBeenCalledWith(env, 123, "owner/repo", 7, "mod:banned", expect.anything());
+    expect(createIssueComment).toHaveBeenCalledWith(env, 123, "owner/repo", 7, expect.stringContaining("Violations older than 1 day(s) no longer count toward this total."));
   });
 
   it("REGRESSION (gate-flagged): a webhook redelivery / queue retry that re-executes the SAME close (same repo+number) does not double-count the violation or escalate past what the single real enforcement action warrants", async () => {
@@ -1672,6 +1686,51 @@ describe("moderation-rules engine escalation (#selfhost-mod-engine)", () => {
     await expect(executeAgentMaintenanceActions(env, ctx({ authorLogin: "farmer99" }), [coupledClose, coupledLabel])).resolves.not.toThrow();
     expect(ensurePullRequestLabel).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "mod:warning", { createMissingLabel: true });
   });
+
+  it("#mod-warning-context: a failed blacklist write degrades to blacklisted:false rather than throwing or claiming a ban that didn't happen", async () => {
+    const env = createTestEnv({});
+    await upsertGlobalModerationConfig(env, { enabled: true, banThreshold: 1 });
+    const originalPrepare = env.DB.prepare.bind(env.DB);
+    vi.spyOn(env.DB, "prepare").mockImplementation((sql: string) => {
+      if (sql.includes("global_contributor_blacklist")) throw new Error("D1 write failed");
+      return originalPrepare(sql);
+    });
+    await expect(executeAgentMaintenanceActions(env, ctx({ authorLogin: "farmer99" }), [coupledClose, coupledLabel])).resolves.not.toThrow();
+    expect(ensurePullRequestLabel).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "mod:banned", { createMissingLabel: true });
+    expect(createIssueComment).toHaveBeenCalledWith(env, 123, "owner/repo", 7, expect.stringContaining("Automatic blacklisting is not enabled"));
+  });
+});
+
+describe("buildModerationEscalationComment (#mod-warning-context)", () => {
+  it("warning tier: states the current count and how many more violations remain before an automatic ban", () => {
+    const text = buildModerationEscalationComment({ tier: "warning", totalCount: 2, banThreshold: 5, violationDecayDays: null, blacklisted: false });
+    expect(text).toBe("This contributor now has 2 recorded moderation violation(s) (warning threshold). 3 more will result in an automatic ban.");
+  });
+
+  it("warning tier: appends the decay-window note when violationDecayDays is configured", () => {
+    const text = buildModerationEscalationComment({ tier: "warning", totalCount: 1, banThreshold: 3, violationDecayDays: 30, blacklisted: false });
+    expect(text).toContain("Violations older than 30 day(s) no longer count toward this total.");
+  });
+
+  it("warning tier: omits the decay-window note when violationDecayDays is null (permanent tally)", () => {
+    const text = buildModerationEscalationComment({ tier: "warning", totalCount: 1, banThreshold: 3, violationDecayDays: null, blacklisted: false });
+    expect(text).not.toContain("no longer count");
+  });
+
+  it("banned tier + blacklisted: reports the threshold reached and that the blacklist entry was added", () => {
+    const text = buildModerationEscalationComment({ tier: "banned", totalCount: 5, banThreshold: 5, violationDecayDays: null, blacklisted: true });
+    expect(text).toBe("This contributor now has 5 recorded moderation violation(s), at or beyond the configured threshold of 5. This contributor has been automatically added to the blacklist.");
+  });
+
+  it("banned tier + NOT blacklisted (autoBlacklistOnBan off or a failed write): reports the threshold reached but that no blacklist entry was added", () => {
+    const text = buildModerationEscalationComment({ tier: "banned", totalCount: 6, banThreshold: 5, violationDecayDays: null, blacklisted: false });
+    expect(text).toBe("This contributor now has 6 recorded moderation violation(s), at or beyond the configured threshold of 5. Automatic blacklisting is not enabled for this instance, so no blacklist entry was added.");
+  });
+
+  it("banned tier: also appends the decay-window note when configured", () => {
+    const text = buildModerationEscalationComment({ tier: "banned", totalCount: 5, banThreshold: 5, violationDecayDays: 14, blacklisted: true });
+    expect(text).toContain("Violations older than 14 day(s) no longer count toward this total.");
+  });
 });
 
 describe("applyModerationEscalationForRule (#review-evasion-protection)", () => {