diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 62950673d..927a52c2f 100644 --- a/src/services/agent-action-executor.ts +++ b/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(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; + 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); } /** diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index da7319404..1f76a6b01 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/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)", () => {