From eba6b84817f3dc25a46e5c1ce2e8db809bdebe77 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 01:33:08 -0700 Subject: [PATCH] feat(agent-actions): add a review-request nagging cooldown (#2463) Throttles a PR/issue author who repeatedly pings @gittensory for review on the same thread: under the configured threshold, pings are just tracked; crossing it applies the repo's policy ("hold" posts a deterministic cooldown reply, "close" additionally closes the PR through the same planner/executor gate stack as blacklist/contributor-cap). Issue threads degrade to "hold" pending a dedicated closeIssue primitive. Off by default, per-repo configurable via .gittensory.yml and a new shared autoCloseExemptLogins list, and scoped to the thread's own author so a third party's pings never affect someone else's PR. Also fixes a pre-existing migration-number collision on main (two 0090 files from separately merged PRs) by renumbering the newer one to 0092. --- .gittensory.yml.example | 15 + apps/gittensory-ui/public/openapi.json | 27 ++ migrations/0091_review_nag_cooldown.sql | 11 + ...092_pull_request_detail_sync_head_sha.sql} | 0 src/db/repositories.ts | 57 +++ src/db/schema.ts | 7 + src/openapi/schemas.ts | 5 + src/queue/processors.ts | 189 ++++++++ src/settings/agent-actions.ts | 47 +- src/settings/auto-close-exempt.ts | 52 +++ src/signals/focus-manifest.ts | 22 + src/types.ts | 27 +- test/unit/agent-actions.test.ts | 57 ++- test/unit/auto-close-exempt.test.ts | 55 +++ test/unit/data-spine.test.ts | 32 ++ test/unit/db-parsers.test.ts | 17 + test/unit/focus-manifest.test.ts | 35 ++ test/unit/queue.test.ts | 410 ++++++++++++++++++ 18 files changed, 1062 insertions(+), 3 deletions(-) create mode 100644 migrations/0091_review_nag_cooldown.sql rename migrations/{0090_pull_request_detail_sync_head_sha.sql => 0092_pull_request_detail_sync_head_sha.sql} (100%) create mode 100644 src/settings/auto-close-exempt.ts create mode 100644 test/unit/auto-close-exempt.test.ts diff --git a/.gittensory.yml.example b/.gittensory.yml.example index 55f6c8bb1f..637c01691a 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -274,3 +274,18 @@ settings: # Label applied to a PR/issue closed for exceeding a cap above. String. Default: over-contributor-limit. # contributorCapLabel: over-contributor-limit + + # Review-request nagging cooldown (#2463, anti-abuse): throttle a non-owner/non-admin/non-bot + # contributor who repeatedly pings @gittensory for review on the same PR/issue. "hold" replies with a + # cooldown notice and takes no further action; "close" closes the PR (issues degrade to "hold" until + # a dedicated closeIssue primitive lands) with a clear reason. Off by default. + # reviewNagPolicy: off # off | hold | close. Default: off. + # reviewNagMaxPings: 3 # Positive integer. Pings above this within the cooldown window trigger the policy. Default: 3. + # reviewNagCooldownDays: 5 # Positive integer. Window the ping count is measured over. Default: 5. + # reviewNagLabel: review-nag-cooldown # Label applied alongside the hold/close action. Default: review-nag-cooldown. + + # Shared repo-scoped exemption list (#2463): GitHub logins never throttled/closed by gittensory's + # deterministic anti-abuse mechanisms (review-nag cooldown today; the per-contributor open-item cap + # above will reuse this list too), on top of the standing owner/admin/automation-bot exemption. + # List of GitHub logins. Default: [] (no additional exemptions). + # autoCloseExemptLogins: [some-trusted-regular] diff --git a/apps/gittensory-ui/public/openapi.json b/apps/gittensory-ui/public/openapi.json index ac5f5c430b..a2eaa0de13 100644 --- a/apps/gittensory-ui/public/openapi.json +++ b/apps/gittensory-ui/public/openapi.json @@ -8602,6 +8602,33 @@ "minimum": 0, "exclusiveMinimum": true }, + "reviewNagPolicy": { + "type": "string", + "enum": [ + "off", + "hold", + "close" + ] + }, + "reviewNagMaxPings": { + "type": "integer", + "minimum": 0, + "exclusiveMinimum": true + }, + "reviewNagCooldownDays": { + "type": "integer", + "minimum": 0, + "exclusiveMinimum": true + }, + "reviewNagLabel": { + "type": "string" + }, + "autoCloseExemptLogins": { + "type": "array", + "items": { + "type": "string" + } + }, "contributorCapLabel": { "type": "string" } diff --git a/migrations/0091_review_nag_cooldown.sql b/migrations/0091_review_nag_cooldown.sql new file mode 100644 index 0000000000..67f11bf25d --- /dev/null +++ b/migrations/0091_review_nag_cooldown.sql @@ -0,0 +1,11 @@ +-- Review-request nagging cooldown (#2463, anti-abuse): throttle a contributor repeatedly pinging @gittensory. +-- Defaults are byte-identical to today: review_nag_policy defaults to 'off' (disabled), so existing rows see no +-- behavior change. review_nag_max_pings / review_nag_cooldown_days / review_nag_label only take effect once a +-- repo opts in by setting the policy to 'hold' or 'close'. +ALTER TABLE repository_settings ADD COLUMN review_nag_policy TEXT NOT NULL DEFAULT 'off'; +ALTER TABLE repository_settings ADD COLUMN review_nag_max_pings INTEGER NOT NULL DEFAULT 3; +ALTER TABLE repository_settings ADD COLUMN review_nag_cooldown_days INTEGER NOT NULL DEFAULT 5; +ALTER TABLE repository_settings ADD COLUMN review_nag_label TEXT NOT NULL DEFAULT 'review-nag-cooldown'; +-- Shared repo-scoped exemption list (#2463): GitHub logins never throttled/closed by gittensory's deterministic +-- anti-abuse mechanisms, on top of the standing owner/admin/automation-bot exemption. Defaults to an empty list. +ALTER TABLE repository_settings ADD COLUMN auto_close_exempt_logins_json TEXT NOT NULL DEFAULT '[]'; diff --git a/migrations/0090_pull_request_detail_sync_head_sha.sql b/migrations/0092_pull_request_detail_sync_head_sha.sql similarity index 100% rename from migrations/0090_pull_request_detail_sync_head_sha.sql rename to migrations/0092_pull_request_detail_sync_head_sha.sql diff --git a/src/db/repositories.ts b/src/db/repositories.ts index eab86a59b3..ef4dd93a82 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -161,6 +161,7 @@ import type { GittensorContributorSnapshot, OfficialGittensorMinerDetection } fr import { classifyMcpClientVersion, LATEST_RECOMMENDED_MCP_VERSION, MINIMUM_SUPPORTED_MCP_VERSION } from "../services/mcp-compatibility"; import { DEFAULT_COMMAND_AUTHORIZATION_POLICY, normalizeCommandAuthorizationPolicy } from "../settings/command-authorization"; import { normalizeContributorBlacklist } from "../settings/contributor-blacklist"; +import { normalizeAutoCloseExemptLogins } from "../settings/auto-close-exempt"; import { normalizeAutonomyPolicy, normalizeAutoMaintainPolicy, DEFAULT_AUTO_MAINTAIN_POLICY } from "../settings/autonomy"; import { decryptSecret, encryptSecret, sha256Hex } from "../utils/crypto"; import { errorMessage, jsonString, nowIso, parseJson, repoParts } from "../utils/json"; @@ -504,6 +505,11 @@ export async function getRepositorySettings(env: Env, fullName: string): Promise contributorOpenPrCap: null, contributorOpenIssueCap: null, contributorCapLabel: "over-contributor-limit", + reviewNagPolicy: "off", + reviewNagMaxPings: 3, + reviewNagCooldownDays: 5, + reviewNagLabel: "review-nag-cooldown", + autoCloseExemptLogins: [], }; } return { @@ -551,6 +557,11 @@ export async function getRepositorySettings(env: Env, fullName: string): Promise contributorOpenPrCap: normalizeOpenItemCap(row.contributorOpenPrCap), contributorOpenIssueCap: normalizeOpenItemCap(row.contributorOpenIssueCap), contributorCapLabel: row.contributorCapLabel, + reviewNagPolicy: normalizeReviewNagPolicy(row.reviewNagPolicy), + reviewNagMaxPings: normalizePositiveIntWithDefault(row.reviewNagMaxPings, 3), + reviewNagCooldownDays: normalizePositiveIntWithDefault(row.reviewNagCooldownDays, 5), + reviewNagLabel: row.reviewNagLabel, + autoCloseExemptLogins: parseAutoCloseExemptLogins(row.autoCloseExemptLoginsJson), createdAt: row.createdAt, updatedAt: row.updatedAt, }; @@ -630,6 +641,11 @@ export async function upsertRepositorySettings(env: Env, settings: Partial 0; } +/** Count-returning variant of {@link hasRecentAuditEvent}, additionally scoped to one `targetKey` (e.g. a single + * `owner/repo#123` PR/issue) rather than the actor's activity across the whole repo. Backs the review-request + * nagging cooldown (#2463): counting how many `@gittensory` pings a contributor has sent on ONE thread within + * the configured cooldown window. */ +export async function countRecentAuditEventsForActorAndTarget(env: Env, actor: string, eventType: string, targetKey: string, sinceIso: string): Promise { + const db = getDb(env.DB); + const [row] = await db + .select({ count: sql`count(*)` }) + .from(auditEvents) + .where(and(eq(auditEvents.actor, actor), eq(auditEvents.eventType, eventType), eq(auditEvents.targetKey, targetKey), gte(auditEvents.createdAt, sinceIso))); + /* v8 ignore next -- count(*) always returns exactly one row; the empty-array guard only satisfies the destructure type. */ + if (!row) return 0; + return row.count; +} + /** Observability for the queue dead-letter rate (#1276): how many jobs (across BOTH the maintenance and webhook * lanes) were dead-lettered since `sinceIso`. Reads the `github_app.dlq_dead_lettered` audit events written by * processDlqBatch — NOT gated behind any review-ops flag, so the infra drop rate is always visible. */ @@ -5642,6 +5683,22 @@ function parseContributorBlacklist(value: string): RepositorySettings["contribut return normalizeContributorBlacklist(parseJson(value, null)).entries; } +function parseAutoCloseExemptLogins(value: string): string[] { + return normalizeAutoCloseExemptLogins(parseJson(value, null)).logins; +} + +function normalizeReviewNagPolicy(value: string | null | undefined): "off" | "hold" | "close" { + return value === "hold" || value === "close" ? value : "off"; +} + +// A review-nag threshold/window is a discrete positive count, not a score — reuses the same non-clamping, +// non-rounding shape as contributorOpenPrCap's normalizeOpenItemCap (#2270): an invalid value (fractional, +// non-positive, non-finite) falls back to the given default rather than being silently coerced. +function normalizePositiveIntWithDefault(value: number | null | undefined, fallback: number): number { + if (typeof value !== "number" || !Number.isFinite(value) || !Number.isInteger(value) || value <= 0) return fallback; + return value; +} + function parseAutonomyPolicy(value: string): AutonomyPolicy { return normalizeAutonomyPolicy(parseJson(value, null)); } diff --git a/src/db/schema.ts b/src/db/schema.ts index 97431c14fd..5f32477b08 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -92,6 +92,13 @@ export const repositorySettings = sqliteTable("repository_settings", { contributorOpenPrCap: integer("contributor_open_pr_cap"), contributorOpenIssueCap: integer("contributor_open_issue_cap"), contributorCapLabel: text("contributor_cap_label").notNull().default("over-contributor-limit"), + // Review-request nagging cooldown (#2463, anti-abuse): default 'off' (disabled). + reviewNagPolicy: text("review_nag_policy").notNull().default("off"), + reviewNagMaxPings: integer("review_nag_max_pings").notNull().default(3), + reviewNagCooldownDays: integer("review_nag_cooldown_days").notNull().default(5), + reviewNagLabel: text("review_nag_label").notNull().default("review-nag-cooldown"), + // Shared repo-scoped exemption list (#2463): a JSON array of GitHub logins. + autoCloseExemptLoginsJson: text("auto_close_exempt_logins_json").notNull().default("[]"), createdAt: text("created_at").notNull().$defaultFn(() => nowIso()), updatedAt: text("updated_at").notNull().$defaultFn(() => nowIso()), }); diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index 880a75af31..f421cd928e 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -633,6 +633,11 @@ export const RepositorySettingsSchema = z contributorOpenPrCap: z.number().int().positive().nullable().optional(), contributorOpenIssueCap: z.number().int().positive().nullable().optional(), contributorCapLabel: z.string().optional(), + reviewNagPolicy: z.enum(["off", "hold", "close"]).optional(), + reviewNagMaxPings: z.number().int().positive().optional(), + reviewNagCooldownDays: z.number().int().positive().optional(), + reviewNagLabel: z.string().optional(), + autoCloseExemptLogins: z.array(z.string()).optional(), createdAt: z.string().nullable().optional(), updatedAt: z.string().nullable().optional(), }) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 2b803e5b78..ae9c2c1fc9 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -48,6 +48,7 @@ import { claimRegateFanoutSlot, recordAgentCommandFeedback, recordAuditEvent, + countRecentAuditEventsForActorAndTarget, recordGateBlockOutcome, getGateBlockOutcome, isGlobalAgentFrozen, @@ -232,6 +233,7 @@ import { planAgentMaintenanceActions, type PlannedAgentAction, } from "../settings/agent-actions"; +import { isAutoCloseExempt } from "../settings/auto-close-exempt"; import { executeAgentMaintenanceActions, executeIssueMaintenanceActions, @@ -3891,6 +3893,24 @@ async function processGitHubWebhook( return; } + // Review-nag cooldown (#2463) runs BEFORE the mention-command dispatch below: a throttled ping must + // short-circuit ahead of the normal answer-card reply, not alongside it. + if ( + eventName === "issue_comment" && + (await maybeThrottleReviewNagPing(env, deliveryId, payload)) + ) { + await recordWebhookEvent(env, { + deliveryId, + eventName, + action: payload.action, + installationId: payload.installation?.id, + repositoryFullName: payload.repository?.full_name, + payloadHash: "processed", + status: "processed", + }); + return; + } + if ( eventName === "issue_comment" && (await maybeProcessGittensoryMentionCommand(env, deliveryId, payload)) @@ -8396,6 +8416,175 @@ async function recloseDisallowedReopenIfNeeded( return true; } +// Audit eventType for one recorded @gittensory ping (#2463). Shared between the recorder below and the +// cooldown-window count query so a naming drift can't silently under/over-count. +const REVIEW_NAG_PING_EVENT_TYPE = "github_app.review_nag_ping"; + +/** + * Review-request nagging cooldown (#2463, anti-abuse): throttle a thread's OWN author repeatedly pinging + * @gittensory for review on the SAME PR/issue. Runs BEFORE maybeProcessGittensoryMentionCommand below so a + * throttled ping short-circuits ahead of the normal answer-card dispatch — under the threshold this just + * records the ping (via the shared audit-events ledger, scoped by targetKey so the count never mixes threads) + * and falls through unchanged; only crossing the threshold applies the repo's configured policy. + * + * Deliberately scoped to the THREAD'S OWN author (`issue.user.login === commenter`): a third party pinging on + * someone else's PR/issue must never throttle or close the AUTHOR's unrelated work — this mirrors the standing + * "never punish someone for another actor's behavior" rule the blacklist/contributor-cap features already + * follow. Off (`reviewNagPolicy: "off"`, the default) is a complete no-op — no audit writes, no reads beyond + * the settings resolve. + */ +async function maybeThrottleReviewNagPing( + env: Env, + deliveryId: string, + payload: GitHubWebhookPayload, +): Promise { + // Only a NEWLY-created comment counts as a ping (mirrors maybeProcessGittensoryMentionCommand) — an edited + // or deleted comment must not re-count or double-count. + if (payload.action !== "created") return false; + const command = parseGittensoryMentionCommand(payload.comment?.body); + if (!command) return false; // not an @gittensory mention at all + const repoFullName = payload.repository?.full_name; + const issue = payload.issue; + const installationId = getInstallationId(payload); + const commenter = payload.comment?.user?.login; + if (!repoFullName || !issue || !installationId || !commenter) return false; + if (payload.comment?.user?.type === "Bot" || /\[bot\]$/i.test(commenter)) return false; + + const settings = await resolveRepositorySettings(env, repoFullName); + /* v8 ignore next -- resolveRepositorySettings always resolves a concrete "off"/"hold"/"close" (NOT NULL DEFAULT 'off'); the undefined side is defensive against the field's optional TS type. */ + const policy = settings.reviewNagPolicy ?? "off"; + if (policy === "off") return false; + + const threadAuthor = issue.user?.login; + if (!threadAuthor || commenter.toLowerCase() !== threadAuthor.toLowerCase()) return false; + + // repoFullName is always "owner/repo" for a real GitHub webhook; the empty-owner fallback only guards a + // malformed/synthetic payload from ever matching an empty commenter login as "the owner". + const repoOwner = repoFullName.includes("/") ? repoFullName.slice(0, repoFullName.indexOf("/")) : ""; + if (commenter.toLowerCase() === repoOwner.toLowerCase()) return false; + if (parseGitHubLoginList(env.ADMIN_GITHUB_LOGINS).has(commenter.toLowerCase())) return false; + // NOTE: no separate isProtectedAutomationAuthor(commenter) check here — every entry in that set (e.g. + // "dependabot[bot]") already ends in "[bot]" and was rejected by the bot-suffix guard above, so it would be + // unreachable dead code at this point (unlike the PR-webhook maintenance path, which checks a PR's stored + // author rather than a live comment author already filtered for bot-ness). + if (isAutoCloseExempt(commenter, settings.autoCloseExemptLogins)) return false; + + const targetKey = `${repoFullName}#${issue.number}`; + /* v8 ignore next -- resolveRepositorySettings always resolves a concrete positive integer (NOT NULL DEFAULT 3); the undefined side is defensive against the field's optional TS type. */ + const maxPings = settings.reviewNagMaxPings ?? 3; + /* v8 ignore next -- resolveRepositorySettings always resolves a concrete positive integer (NOT NULL DEFAULT 5); the undefined side is defensive against the field's optional TS type. */ + const cooldownDays = settings.reviewNagCooldownDays ?? 5; + const sinceIso = new Date(Date.now() - cooldownDays * 24 * 60 * 60 * 1000).toISOString(); + const priorPings = await countRecentAuditEventsForActorAndTarget(env, commenter, REVIEW_NAG_PING_EVENT_TYPE, targetKey, sinceIso); + const pingCount = priorPings + 1; // this ping counts too + + // Always record the ping first so the running count reflects reality even when the rest of this handler + // short-circuits below (a failed recordAuditEvent must never block the mention-command fallthrough). + await recordAuditEvent(env, { + eventType: REVIEW_NAG_PING_EVENT_TYPE, + actor: commenter, + targetKey, + outcome: "completed", + detail: `ping ${pingCount}/${maxPings} within ${cooldownDays}d window`, + metadata: { deliveryId, repoFullName }, + }).catch( + /* v8 ignore next -- fail-safe: an audit write failure never blocks the mention-command fallthrough */ + () => undefined, + ); + + if (pingCount <= maxPings) return false; // under threshold — normal command processing proceeds unchanged + + const mode = resolveAgentActionMode({ + globalPaused: isGlobalAgentPause(env) || (await isGlobalAgentFrozen(env)), + agentPaused: settings.agentPaused, + agentDryRun: settings.agentDryRun, + }); + + // "close" only ever applies to a PR thread — an issue thread has no closeIssue primitive yet (tracked + // separately), so it degrades to "hold" with a comment explaining the v1 limit. + if (policy === "hold" || !issue.pull_request) { + if (mode === "live") { + await createIssueComment( + env, + installationId, + repoFullName, + issue.number, + `@${commenter} this thread has reached the review-request cooldown limit (${maxPings} pings within ${cooldownDays} days). Please wait for the cooldown window to pass before pinging @gittensory again. This is an automated maintenance action.`, + ).catch( + /* v8 ignore next -- fail-safe: a comment-post failure must not crash the throttle decision itself */ + () => undefined, + ); + } + await recordAuditEvent(env, { + eventType: "github_app.review_nag_cooldown_applied", + actor: "gittensory", + targetKey, + outcome: mode === "live" ? "completed" : "denied", + detail: `hold applied: ${commenter} pinged ${pingCount} times (limit ${maxPings})`, + metadata: { deliveryId, repoFullName, mode, policy }, + }).catch( + /* v8 ignore next -- fail-safe: an audit write failure never blocks the handler */ + () => undefined, + ); + return true; // short-circuit — skip the normal @gittensory command dispatch + } + + // policy === "close" on a PR thread: build the deterministic label+close plan through the SAME planner/ + // executor gate stack (autonomy/dry-run/kill-switch/write-permission) as every other agent-driven mutation. + const pr = await getPullRequest(env, repoFullName, issue.number); + if (!pr || pr.state !== "open") return false; // nothing left to close — fall through harmlessly + + const planned = planAgentMaintenanceActions({ + conclusion: "skipped", + blockerTitles: [], + autonomy: settings.autonomy, + changedPaths: [], + hardGuardrailGlobs: [], + authorIsOwner: false, + authorIsAdmin: false, + authorIsAutomationBot: false, + ciState: "unverified", + reviewNagMatch: { matched: true, authorLogin: commenter, pingCount, maxPings }, + // planAgentMaintenanceActions applies its own DEFAULT_REVIEW_NAG_LABEL fallback for an absent label — + // mirrors how blacklistLabel is threaded straight through without a second fallback layer here. + reviewNagLabel: settings.reviewNagLabel, + pr: { labels: pr.labels, headSha: pr.headSha }, + }); + if (planned.length === 0) { + // Autonomy is not currently acting for label/close — nothing to execute, but the policy still engaged. + await recordAuditEvent(env, { + eventType: "github_app.review_nag_cooldown_applied", + actor: "gittensory", + targetKey, + outcome: "denied", + detail: `close policy engaged but autonomy is not acting for label/close: ${commenter} pinged ${pingCount} times (limit ${maxPings})`, + metadata: { deliveryId, repoFullName, mode, policy }, + }).catch( + /* v8 ignore next -- fail-safe: an audit write failure never blocks the handler */ + () => undefined, + ); + return true; + } + + const installation = await getInstallation(env, installationId); + await executeAgentMaintenanceActions( + env, + { + installationId, + repoFullName, + pullNumber: pr.number, + headSha: pr.headSha, + autonomy: settings.autonomy, + agentPaused: settings.agentPaused, + agentDryRun: settings.agentDryRun, + installationPermissions: installation?.permissions ?? null, + authorLogin: pr.authorLogin, + }, + planned, + ); + return true; +} + async function maybeProcessGittensoryMentionCommand( env: Env, deliveryId: string, diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 41ad099963..2710fb2138 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -27,6 +27,10 @@ export const DEFAULT_BLACKLIST_LABEL = "slop"; // configurable-with-fallback shape as DEFAULT_BLACKLIST_LABEL — a repo can override it via // `.gittensory.yml` (`settings.contributorCapLabel`); this is only the fallback when unset. export const DEFAULT_CONTRIBUTOR_CAP_LABEL = "over-contributor-limit"; +// Default label applied to a PR closed for review-nag cooldown (#2463). NOT hardcoded into the action — it is +// configurable per-repo via `.gittensory.yml` (`settings.reviewNagLabel`); the planner uses the resolved label +// and falls back to this default, mirroring DEFAULT_BLACKLIST_LABEL's shape. +export const DEFAULT_REVIEW_NAG_LABEL = "review-nag-cooldown"; // A PR that PASSES the gate but touches a hard-guardrail path is NOT ready to auto-merge — it is withheld // for a human (the merge/approve/close dispositions are suppressed below). Labeling it `ready-to-merge` // would be misleading (the label promises an auto-merge that never happens), so a guarded passing PR gets @@ -65,7 +69,7 @@ export type PlannedAgentAction = { // duplicate / slop / CI). The breaker downgrades ONLY "heuristic" closes; the deterministic close is EXEMPT // (silently holding a close whose comment already promised closure would be incoherent). Absent on non-close // actions; treated as a heuristic close only when explicitly tagged "heuristic". - closeKind?: "linked-issue-hard-rule" | "blacklist" | "contributor_cap" | "heuristic"; + closeKind?: "linked-issue-hard-rule" | "blacklist" | "contributor_cap" | "review_nag" | "heuristic"; // For a CI-driven heuristic close, the CI state that must still hold at actuation time. Other heuristic // closes (gate verdict, duplicate/slop, conflict) do not depend on red CI and must not be blocked by green CI. // ALWAYS set for a heuristic close (never omitted) -- see the field's doc comment on AgentPendingActionParams @@ -162,6 +166,17 @@ export type AgentActionPlanInput = { // The repo-configured label applied to an over-cap author's PR/issue (#2270), resolved from `.gittensory.yml`. // Absent ⇒ the default (`DEFAULT_CONTRIBUTOR_CAP_LABEL` = "over-contributor-limit"). contributorCapLabel?: string | undefined; + // Review-nag cooldown (#2463, anti-abuse): when the PR author has pinged `@gittensory` past the repo's + // configured threshold within the cooldown window AND the repo's `reviewNagPolicy` is `"close"`, the + // disposition SHORT-CIRCUITS to a deterministic label + close ahead of ALL merit/CI/AI analysis — same + // zero-hallucination shape as blacklistMatch, so its close is tagged `closeKind: "review_nag"`. Fires for a + // CONTRIBUTOR only (owner/admin/automation bots are never auto-closed). The comment-throttle decision itself + // (counting pings, choosing hold vs. close) happens at the webhook trigger, not here — this input is already + // the resolved "yes, close this PR" verdict. Absent / not-matched ⇒ no effect. + reviewNagMatch?: { matched: boolean; authorLogin: string; pingCount: number; maxPings: number } | undefined; + // The repo-configured label applied to a review-nag-closed PR (#2463), resolved from `.gittensory.yml`. + // Absent ⇒ the default (`DEFAULT_REVIEW_NAG_LABEL` = "review-nag-cooldown"). + reviewNagLabel?: string | undefined; // Flag-then-close double-check for the linked-issue hard rule (#linked-issue-verify-before-close). When // `verifyBeforeClose` is true (the default), a violation FLAGS the PR (pending-closure label + warning comment) // on first detection and only CLOSES on a LATER evaluation when the violation STILL holds AND the PR already @@ -282,6 +297,13 @@ function contributorCapCloseMessage(authorLogin: string, openCount: number, cap: return `Gittensory closed this because @${authorLogin} has ${openCount} open ${itemNoun}, above this repository's configured limit of ${cap}. Close or merge an existing one to open a new one. This is an automated maintenance action.`; } +// The close comment for review-nag cooldown (#2463). DOES interpolate authorLogin/pingCount/maxPings — none of +// that is private (the author's own login and their own public @gittensory ping count are already public/ +// derivable from the PR thread itself), mirroring the contributor-cap close message's same reasoning. +function reviewNagCloseMessage(authorLogin: string, pingCount: number, maxPings: number): string { + return `Gittensory closed this because @${authorLogin} pinged @gittensory ${pingCount} times, above this repository's configured limit of ${maxPings}. Please wait for the cooldown window to pass before requesting review again. This is an automated maintenance action.`; +} + /** * Plan the maintainer auto-maintain actions for one PR. Returns a COHERENT set (never both approve and * request-changes; never both merge and close), each entry already filtered to an acting autonomy class. @@ -347,6 +369,29 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne return actions; } + // Review-nag cooldown (#2463): same zero-hallucination short-circuit shape as the blacklist above — fires + // ahead of ALL merit/CI/AI analysis, for a CONTRIBUTOR only. The webhook trigger has already decided "this + // ping crosses the threshold AND the repo's policy is close" before ever setting this input; the planner's + // only job is to build the deterministic label+close plan under the repo's normal autonomy/dry-run/kill-switch + // gates, exactly like every other action. + const reviewNagContributor = !input.authorIsOwner && !input.authorIsAdmin && !input.authorIsAutomationBot; + if (input.reviewNagMatch?.matched === true && reviewNagContributor) { + const { authorLogin, pingCount, maxPings } = input.reviewNagMatch; + const label = input.reviewNagLabel ?? DEFAULT_REVIEW_NAG_LABEL; + if (acting("label")) actions.push({ actionClass: "label", requiresApproval: approval("label"), reason: "review-nag cooldown", label, labelOp: "add" }); + if (acting("close")) { + actions.push({ + actionClass: "close", + requiresApproval: approval("close"), + reason: "review-nag cooldown", + closeComment: sanitizePublicComment(reviewNagCloseMessage(authorLogin, pingCount, maxPings)), + closeKind: "review_nag", + ...(input.pr.headSha ? { expectedHeadSha: input.pr.headSha } : {}), + }); + } + return actions; + } + // Only a SKIPPED gate (genuinely not evaluated) drives no action. A NEUTRAL gate (first-time-contributor // grace, or eval-not-ready while state is still syncing) is gate-NON-BLOCKING: it flows to the disposition so // the PR is merged (clean+green) or HELD with a label — never left silently undecided. (#harm-stop neutral-silent-stuck) diff --git a/src/settings/auto-close-exempt.ts b/src/settings/auto-close-exempt.ts new file mode 100644 index 0000000000..4e2aaa06de --- /dev/null +++ b/src/settings/auto-close-exempt.ts @@ -0,0 +1,52 @@ +// Shared repo-scoped exemption list (#2463) for gittensory's deterministic anti-abuse auto-close/throttle +// mechanisms — currently the review-nag cooldown; intended to be reused by the per-contributor open-item cap +// (#2270) once that lands, rather than each feature growing its own duplicate whitelist. A maintainer-named +// GitHub login here is NEVER throttled or closed by either mechanism, on top of the standing owner/admin/ +// automation-bot exemption every such mechanism already honors. Config-driven and layered the same as other +// settings (`.gittensory.yml` > DB), never hard-coded for any repo. Mirrors contributor-blacklist.ts's shape +// (normalize → validated list + warnings), minus the reason/evidence metadata a ban carries that an exemption +// doesn't need. +const GITHUB_LOGIN = /^[a-zA-Z0-9](?:[a-zA-Z0-9]|-(?=[a-zA-Z0-9])){0,38}$/; +const MAX_ENTRIES = 500; + +/** Normalize a raw exempt-logins value (DB JSON or `.gittensory.yml`) into a validated, de-duplicated list of + * GitHub logins. Never throws: malformed entries are dropped with a warning. De-dup is case-insensitive (the + * FIRST occurrence's casing is kept). */ +export function normalizeAutoCloseExemptLogins(input: unknown): { logins: string[]; warnings: string[] } { + const warnings: string[] = []; + if (input === undefined || input === null) return { logins: [], warnings }; + if (!Array.isArray(input)) { + warnings.push("autoCloseExemptLogins must be a list of GitHub logins; ignoring it."); + return { logins: [], warnings }; + } + const logins: string[] = []; + const seen = new Set(); + for (const [index, raw] of input.entries()) { + if (logins.length >= MAX_ENTRIES) { + warnings.push(`autoCloseExemptLogins is capped at ${MAX_ENTRIES} entries; dropping the rest.`); + break; + } + if (typeof raw !== "string") { + warnings.push(`autoCloseExemptLogins[${index}] must be a string login; ignoring it.`); + continue; + } + const login = raw.trim(); + if (!GITHUB_LOGIN.test(login)) { + warnings.push(`autoCloseExemptLogins[${index}] is not a valid GitHub login; ignoring it.`); + continue; + } + const key = login.toLowerCase(); + if (seen.has(key)) continue; // first occurrence wins + seen.add(key); + logins.push(login); + } + return { logins, warnings }; +} + +/** Case-insensitive membership check against the resolved exempt-logins list. Absent/empty list ⇒ never exempt + * (the safe default — an unconfigured repo exempts no one beyond the standing owner/admin/bot rule). */ +export function isAutoCloseExempt(login: string | null | undefined, exemptLogins: readonly string[] | undefined): boolean { + if (!login) return false; + const lower = login.toLowerCase(); + return (exemptLogins ?? []).some((entry) => entry.toLowerCase() === lower); +} diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 63d8566c65..80c102524d 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -3,6 +3,7 @@ import type { GatePolicyPack, GateRuleMode, JsonValue, RepositorySettings } from import { normalizeAutonomyPolicy, normalizeAutoMaintainPolicy } from "../settings/autonomy"; import { normalizeCommandAuthorizationPolicy } from "../settings/command-authorization"; import { mergeContributorBlacklists, normalizeContributorBlacklist } from "../settings/contributor-blacklist"; +import { normalizeAutoCloseExemptLogins } from "../settings/auto-close-exempt"; import { hasUnsafeWildcardCount } from "./change-guardrail"; import { PUBLIC_LOCAL_PATH_INLINE } from "./redaction"; @@ -128,6 +129,11 @@ export type FocusManifestSettings = Partial< | "contributorOpenPrCap" | "contributorOpenIssueCap" | "contributorCapLabel" + | "reviewNagPolicy" + | "reviewNagMaxPings" + | "reviewNagCooldownDays" + | "reviewNagLabel" + | "autoCloseExemptLogins" > >; @@ -818,6 +824,22 @@ function parseSettingsOverride(value: JsonValue | undefined, warnings: string[]) } const contributorCapLabel = normalizeOptionalString(r.contributorCapLabel, "settings.contributorCapLabel", warnings); if (contributorCapLabel !== null) out.contributorCapLabel = contributorCapLabel; + // Review-request nagging cooldown (#2463): throttle a contributor repeatedly pinging @gittensory for review. + const reviewNagPolicy = normalizeOptionalEnum(r.reviewNagPolicy, "settings.reviewNagPolicy", ["off", "hold", "close"] as const, warnings); + if (reviewNagPolicy !== null) out.reviewNagPolicy = reviewNagPolicy; + const reviewNagMaxPings = normalizeOptionalPositiveInteger(r.reviewNagMaxPings, "settings.reviewNagMaxPings", warnings); + if (reviewNagMaxPings !== null) out.reviewNagMaxPings = reviewNagMaxPings; + const reviewNagCooldownDays = normalizeOptionalPositiveInteger(r.reviewNagCooldownDays, "settings.reviewNagCooldownDays", warnings); + if (reviewNagCooldownDays !== null) out.reviewNagCooldownDays = reviewNagCooldownDays; + const reviewNagLabel = normalizeOptionalString(r.reviewNagLabel, "settings.reviewNagLabel", warnings); + if (reviewNagLabel !== null) out.reviewNagLabel = reviewNagLabel; + // Shared repo-scoped exemption list (#2463): only set it when at least one VALID login survives + // normalization, so a malformed block never blanks the DB-configured list via the resolver's overlay. + if (r.autoCloseExemptLogins !== undefined) { + const { logins, warnings: exemptWarnings } = normalizeAutoCloseExemptLogins(r.autoCloseExemptLogins); + warnings.push(...exemptWarnings); + if (logins.length > 0) out.autoCloseExemptLogins = logins; + } return out; } diff --git a/src/types.ts b/src/types.ts index d6a75571f1..608c0e6a4e 100644 --- a/src/types.ts +++ b/src/types.ts @@ -626,6 +626,31 @@ export type RepositorySettings = { * disposition works regardless of the label a repo sets. Always populated by the DB layer; optional so * existing settings fixtures/callers need not be touched. */ contributorCapLabel?: string | undefined; + /** Review-request nagging cooldown (#2463, anti-abuse): throttle a contributor repeatedly pinging + * `@gittensory` (any command) on this repo. `"off"` (default) is a no-op; `"hold"` posts a deterministic + * cooldown reply and takes no further action; `"close"` additionally closes the thread (PR threads only in + * v1 — a plain issue thread degrades to `"hold"` behavior until #2493's `closeIssue` primitive lands). + * Always populated by the DB layer (default `"off"`); optional so existing settings fixtures/callers need + * not be touched. */ + reviewNagPolicy?: "off" | "hold" | "close" | undefined; + /** Review-nag cooldown (#2463): how many `@gittensory` pings a contributor may make on this repo within + * {@link reviewNagCooldownDays} before the (N+1)th is throttled. Always populated by the DB layer (default + * `3`); optional so existing settings fixtures/callers need not be touched. Only meaningful when + * {@link reviewNagPolicy} is not `"off"`. */ + reviewNagMaxPings?: number | undefined; + /** Review-nag cooldown (#2463): the rolling window (in days) {@link reviewNagMaxPings} counts against. Always + * populated by the DB layer (default `5`); optional so existing settings fixtures/callers need not be + * touched. */ + reviewNagCooldownDays?: number | undefined; + /** The label applied to a thread closed for review-nag cooldown (#2463), mirroring {@link blacklistLabel}'s + * configurable-with-fallback shape. Always populated by the DB layer (default `"review-nag-cooldown"`); + * optional so existing settings fixtures/callers need not be touched. */ + reviewNagLabel?: string | undefined; + /** Shared repo-scoped exemption list (#2463, anti-abuse): GitHub logins that are NEVER throttled or closed by + * gittensory's deterministic anti-abuse mechanisms (review-nag and the per-contributor open-item cap above), + * on top of the standing owner/admin/automation-bot exemption. Always populated by the DB layer (default + * `[]`); optional so existing settings fixtures/callers need not be touched. */ + autoCloseExemptLogins?: string[] | undefined; /** Agent-layer autonomy dial (#773): per-action-class level. Always populated by the DB layer (default * `{}` = deny-by-default = "observe" for every class); optional so existing settings fixtures/callers * need not be touched. The single source the action layer (#778) reads via `resolveAutonomy`. */ @@ -701,7 +726,7 @@ export type AgentPendingActionParams = { // (#2127), and the actuation-time live-CI re-check (#2364) — which only applies to a heuristic close — still // fires correctly once the row is replayed through pendingActionToPlanned, rather than silently skipping for // a lost discriminator. - closeKind?: "linked-issue-hard-rule" | "blacklist" | "contributor_cap" | "heuristic"; + closeKind?: "linked-issue-hard-rule" | "blacklist" | "contributor_cap" | "review_nag" | "heuristic"; // For a CI-driven heuristic close, persist the CI state that must still hold when the staged action replays // (#2364). This is separate from closeKind because heuristic closes also cover non-CI adverse signals. // ALWAYS set (to "failed" or "not_required") for a freshly planned heuristic close (#2478) -- never omitted -- diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 377b32fa46..a786f9d154 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { AGENT_LABEL_CHANGES, AGENT_LABEL_NEEDS_REVIEW, AGENT_LABEL_READY, DEFAULT_BLACKLIST_LABEL, DEFAULT_CONTRIBUTOR_CAP_LABEL, downgradeCloseToHold, downgradeMergeToHold, isProtectedAutomationAuthor, planAgentMaintenanceActions, type AgentActionPlanInput, type PlannedAgentAction } from "../../src/settings/agent-actions"; +import { AGENT_LABEL_CHANGES, AGENT_LABEL_NEEDS_REVIEW, AGENT_LABEL_READY, DEFAULT_BLACKLIST_LABEL, DEFAULT_CONTRIBUTOR_CAP_LABEL, DEFAULT_REVIEW_NAG_LABEL, downgradeCloseToHold, downgradeMergeToHold, isProtectedAutomationAuthor, planAgentMaintenanceActions, type AgentActionPlanInput, type PlannedAgentAction } from "../../src/settings/agent-actions"; import { AGENT_LABEL_PENDING_CLOSURE } from "../../src/review/linked-issue-hard-rules"; import type { GateCheckConclusion } from "../../src/rules/advisory"; @@ -928,3 +928,58 @@ describe("per-contributor open-item cap short-circuit (#2270)", () => { expect(plan[1]).toMatchObject({ closeKind: "blacklist" }); }); }); + +describe("review-nag cooldown short-circuit (#2463)", () => { + const nagged = (extra: Partial = {}) => + input({ + conclusion: "success", + autonomy: { label: "auto", close: "auto", approve: "auto", merge: "auto" }, + reviewNagMatch: { matched: true, authorLogin: "chatty-contributor", pingCount: 4, maxPings: 3 }, + ...extra, + }); + + it("labels + closes a nagging contributor's PR, winning over a passing gate (no merit review / merge)", () => { + const plan = planAgentMaintenanceActions(nagged()); + expect(classes(plan)).toEqual(["label", "close"]); // short-circuit: no approve/merge despite a SUCCESS gate + expect(plan[0]).toMatchObject({ actionClass: "label", label: DEFAULT_REVIEW_NAG_LABEL, labelOp: "add" }); + expect(plan[1]).toMatchObject({ actionClass: "close", closeKind: "review_nag" }); + expect(plan[1]?.closeComment).toContain("chatty-contributor"); + expect(plan[1]?.closeComment).toContain("4"); + expect(plan[1]?.closeComment).toContain("3"); + }); + + it("pins the review-nag close to the reviewed head, mirroring blacklist/merge/approve", () => { + const plan = planAgentMaintenanceActions(nagged({ pr: { labels: [], headSha: "h-reviewed" } })); + expect(plan.find((a) => a.actionClass === "close")).toMatchObject({ closeKind: "review_nag", expectedHeadSha: "h-reviewed" }); + }); + + it("omits expectedHeadSha on the review-nag close when the PR record has no headSha (defensive fallback)", () => { + const plan = planAgentMaintenanceActions(nagged()); + expect(plan.find((a) => a.actionClass === "close")?.expectedHeadSha).toBeUndefined(); + }); + + it("uses the repo-configured reviewNagLabel, defaulting to 'review-nag-cooldown' when unset", () => { + expect(planAgentMaintenanceActions(nagged({ reviewNagLabel: "cooldown-hit" }))[0]).toMatchObject({ label: "cooldown-hit" }); + expect(DEFAULT_REVIEW_NAG_LABEL).toBe("review-nag-cooldown"); + expect(planAgentMaintenanceActions(nagged())[0]).toMatchObject({ label: "review-nag-cooldown" }); + }); + + it("fires AHEAD of CI — closes even while CI is still pending (not the pending early-return)", () => { + expect(classes(planAgentMaintenanceActions(nagged({ ciState: "pending" })))).toEqual(["label", "close"]); + }); + + it("NEVER fires for the owner, an admin login, or an automation bot (standing rule) — the PR falls through to normal disposition", () => { + expect(classes(planAgentMaintenanceActions(nagged({ authorIsOwner: true })))).not.toContain("close"); + expect(classes(planAgentMaintenanceActions(nagged({ authorIsAdmin: true })))).not.toContain("close"); + expect(classes(planAgentMaintenanceActions(nagged({ authorIsAutomationBot: true })))).not.toContain("close"); + }); + + it("no-ops when the match is not matched (normal disposition runs)", () => { + expect(classes(planAgentMaintenanceActions(nagged({ reviewNagMatch: { matched: false, authorLogin: "x", pingCount: 0, maxPings: 3 } })))).not.toContain("close"); + }); + + it("respects autonomy: observe plans nothing (still short-circuits); label-only labels but does not close", () => { + expect(planAgentMaintenanceActions(nagged({ autonomy: {} }))).toEqual([]); + expect(classes(planAgentMaintenanceActions(nagged({ autonomy: { label: "auto" } })))).toEqual(["label"]); + }); +}); diff --git a/test/unit/auto-close-exempt.test.ts b/test/unit/auto-close-exempt.test.ts new file mode 100644 index 0000000000..ff6de0bb28 --- /dev/null +++ b/test/unit/auto-close-exempt.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, it } from "vitest"; +import { isAutoCloseExempt, normalizeAutoCloseExemptLogins } from "../../src/settings/auto-close-exempt"; + +describe("normalizeAutoCloseExemptLogins (#2463)", () => { + it("returns [] for null/undefined and a non-array (with a warning)", () => { + expect(normalizeAutoCloseExemptLogins(undefined).logins).toEqual([]); + expect(normalizeAutoCloseExemptLogins(null).logins).toEqual([]); + const notArray = normalizeAutoCloseExemptLogins({ login: "x" }); + expect(notArray.logins).toEqual([]); + expect(notArray.warnings[0]).toMatch(/must be a list/); + }); + + it("accepts valid GitHub logins (alnum, single internal hyphen, ≤39 chars)", () => { + const { logins } = normalizeAutoCloseExemptLogins(["a-b", "user123", "a".repeat(39)]); + expect(logins).toEqual(["a-b", "user123", "a".repeat(39)]); + }); + + it("drops non-string and invalid-login entries with a warning", () => { + const { logins, warnings } = normalizeAutoCloseExemptLogins([42, "-bad", "bad-", "a--b", "has space", "a".repeat(40)]); + expect(logins).toEqual([]); + expect(warnings.length).toBeGreaterThanOrEqual(5); + }); + + it("trims whitespace around a login", () => { + const { logins } = normalizeAutoCloseExemptLogins([" spaced-login "]); + expect(logins).toEqual(["spaced-login"]); + }); + + it("de-duplicates by case-insensitive login, keeping the FIRST occurrence's casing", () => { + const { logins } = normalizeAutoCloseExemptLogins(["Mona", "mona"]); + expect(logins).toEqual(["Mona"]); + }); + + it("caps the list and warns when over the limit", () => { + const many = Array.from({ length: 505 }, (_, i) => `user${i}`); + const { logins, warnings } = normalizeAutoCloseExemptLogins(many); + expect(logins).toHaveLength(500); + expect(warnings.some((w) => w.includes("capped"))).toBe(true); + }); +}); + +describe("isAutoCloseExempt (#2463)", () => { + it("matches case-insensitively", () => { + expect(isAutoCloseExempt("mona", ["Mona", "octocat"])).toBe(true); + expect(isAutoCloseExempt("OCTOCAT", ["Mona", "octocat"])).toBe(true); + }); + + it("returns false for a non-match, a missing login, or an absent/empty list", () => { + expect(isAutoCloseExempt("stranger", ["Mona"])).toBe(false); + expect(isAutoCloseExempt(null, ["Mona"])).toBe(false); + expect(isAutoCloseExempt(undefined, ["Mona"])).toBe(false); + expect(isAutoCloseExempt("anyone", undefined)).toBe(false); + expect(isAutoCloseExempt("anyone", [])).toBe(false); + }); +}); diff --git a/test/unit/data-spine.test.ts b/test/unit/data-spine.test.ts index 8f997ab45b..c9d5955f62 100644 --- a/test/unit/data-spine.test.ts +++ b/test/unit/data-spine.test.ts @@ -324,6 +324,38 @@ describe("data spine repositories", () => { expect((await getRepositorySettings(env, "owner/caprepo")).contributorCapLabel).toBe("spam-cap"); await upsertRepositorySettings(env, { repoFullName: "owner/caprepo", contributorCapLabel: "renamed-cap" }); expect((await getRepositorySettings(env, "owner/caprepo")).contributorCapLabel).toBe("renamed-cap"); // update persists + // #2463 review-nag cooldown + shared exemption list: no row and no override both default to off/3/5/the + // default label/empty exemption list. + expect(await getRepositorySettings(env, "missing/repo")).toMatchObject({ + reviewNagPolicy: "off", + reviewNagMaxPings: 3, + reviewNagCooldownDays: 5, + reviewNagLabel: "review-nag-cooldown", + autoCloseExemptLogins: [], + }); + expect(await getRepositorySettings(env, "owner/defaultpack")).toMatchObject({ reviewNagPolicy: "off", autoCloseExemptLogins: [] }); + // Round-trips on insert and persists on update. + await upsertRepositorySettings(env, { + repoFullName: "owner/nagrepo", + reviewNagPolicy: "close", + reviewNagMaxPings: 5, + reviewNagCooldownDays: 10, + reviewNagLabel: "too-many-pings", + autoCloseExemptLogins: ["Trusted-Regular"], + }); + expect(await getRepositorySettings(env, "owner/nagrepo")).toMatchObject({ + reviewNagPolicy: "close", + reviewNagMaxPings: 5, + reviewNagCooldownDays: 10, + reviewNagLabel: "too-many-pings", + autoCloseExemptLogins: ["Trusted-Regular"], + }); + await upsertRepositorySettings(env, { repoFullName: "owner/nagrepo", reviewNagPolicy: "hold", autoCloseExemptLogins: [] }); + expect(await getRepositorySettings(env, "owner/nagrepo")).toMatchObject({ reviewNagPolicy: "hold", autoCloseExemptLogins: [] }); // update persists + can clear + // An invalid policy string is dropped to "off"; a non-positive/fractional ping count or cooldown falls + // back to its default rather than being silently coerced. + await upsertRepositorySettings(env, { repoFullName: "owner/badnagrepo", reviewNagPolicy: "delete-everything" as never, reviewNagMaxPings: -1, reviewNagCooldownDays: 2.5 as never }); + expect(await getRepositorySettings(env, "owner/badnagrepo")).toMatchObject({ reviewNagPolicy: "off", reviewNagMaxPings: 3, reviewNagCooldownDays: 5 }); expect(updated.slopAiAdvisory).toBe(false); expect(await getRepoSyncState(env, "missing/repo")).toBeNull(); expect(await getPullRequest(env, "owner/repo", 404)).toBeNull(); diff --git a/test/unit/db-parsers.test.ts b/test/unit/db-parsers.test.ts index b1bd5d7192..58d729c88f 100644 --- a/test/unit/db-parsers.test.ts +++ b/test/unit/db-parsers.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { claimRegateFanoutSlot, countRecentDeadLetters, + countRecentAuditEventsForActorAndTarget, getLatestScorePreview, getRepoAuthorPullRequestHistory, getLatestScoringModelSnapshot, @@ -372,6 +373,22 @@ describe("database row parser hardening", () => { expect(await countRecentDeadLetters(env, "2026-06-24T13:00:00.000Z")).toBe(0); // none after the cutoff → count(*) returns 0 }); + it("countRecentAuditEventsForActorAndTarget counts events scoped to ONE actor+eventType+targetKey since a cutoff (#2463)", async () => { + const env = createTestEnv(); + await recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "owner/repo#1", outcome: "completed", createdAt: "2026-06-24T10:00:00.000Z" }); + await recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "owner/repo#1", outcome: "completed", createdAt: "2026-06-24T12:00:00.000Z" }); + // A different actor on the SAME target must not be counted (the actor filter). + await recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "someone-else", targetKey: "owner/repo#1", outcome: "completed", createdAt: "2026-06-24T12:00:00.000Z" }); + // The SAME actor pinging a DIFFERENT PR/issue must not be counted (the targetKey filter). + await recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "owner/repo#2", outcome: "completed", createdAt: "2026-06-24T12:00:00.000Z" }); + // An unrelated event type on the same actor+target must not be counted (the eventType filter). + await recordAuditEvent(env, { eventType: "github_app.agent_command_replied", actor: "chatty", targetKey: "owner/repo#1", outcome: "completed", createdAt: "2026-06-24T12:00:00.000Z" }); + + expect(await countRecentAuditEventsForActorAndTarget(env, "chatty", "github_app.review_nag_ping", "owner/repo#1", "2026-06-24T09:00:00.000Z")).toBe(2); + expect(await countRecentAuditEventsForActorAndTarget(env, "chatty", "github_app.review_nag_ping", "owner/repo#1", "2026-06-24T11:00:00.000Z")).toBe(1); // only the 12:00 one + expect(await countRecentAuditEventsForActorAndTarget(env, "chatty", "github_app.review_nag_ping", "owner/repo#1", "2026-06-24T13:00:00.000Z")).toBe(0); // none after the cutoff → count(*) returns 0 + }); + it("computes complete case-insensitive repo author PR history for gate grace", async () => { const env = createTestEnv(); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 6672bbc824..b8f8c757aa 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -1344,6 +1344,41 @@ describe("parseFocusManifest settings override + resolveEffectiveSettings", () = expect(nonNumber.settings.contributorOpenPrCap).toBeUndefined(); }); + it("parses + resolves the review-nag cooldown settings from the settings: block, overlaying the DB (#2463)", () => { + const manifest = parseFocusManifest({ settings: { reviewNagPolicy: "close", reviewNagMaxPings: 5, reviewNagCooldownDays: 10, reviewNagLabel: "too-chatty" } }); + expect(manifest.settings.reviewNagPolicy).toBe("close"); + expect(manifest.settings.reviewNagMaxPings).toBe(5); + expect(manifest.settings.reviewNagCooldownDays).toBe(10); + expect(manifest.settings.reviewNagLabel).toBe("too-chatty"); + // yml overlays a DB-configured policy. + const eff = resolveEffectiveSettings({ reviewNagPolicy: "off", reviewNagMaxPings: 3, reviewNagCooldownDays: 5, reviewNagLabel: "review-nag-cooldown" } as unknown as RepositorySettings, manifest); + expect(eff.reviewNagPolicy).toBe("close"); + expect(eff.reviewNagMaxPings).toBe(5); + // Omitted in yml ⇒ the DB-configured policy survives untouched. + const noOverride = resolveEffectiveSettings({ reviewNagPolicy: "hold", reviewNagMaxPings: 7 } as unknown as RepositorySettings, parseFocusManifest({})); + expect(noOverride.reviewNagPolicy).toBe("hold"); + expect(noOverride.reviewNagMaxPings).toBe(7); + // An invalid policy enum / non-positive ping count / non-positive cooldown is dropped with a warning + // rather than silently coerced. + const invalid = parseFocusManifest({ settings: { reviewNagPolicy: "delete-everything" as never, reviewNagMaxPings: 0, reviewNagCooldownDays: -1 } }); + expect(invalid.settings.reviewNagPolicy).toBeUndefined(); + expect(invalid.settings.reviewNagMaxPings).toBeUndefined(); + expect(invalid.settings.reviewNagCooldownDays).toBeUndefined(); + expect(invalid.warnings.some((w) => /settings\.reviewNagPolicy/.test(w))).toBe(true); + expect(invalid.warnings.some((w) => /settings\.reviewNagMaxPings/.test(w))).toBe(true); + expect(invalid.warnings.some((w) => /settings\.reviewNagCooldownDays/.test(w))).toBe(true); + }); + + it("parses + resolves autoCloseExemptLogins from the settings: block, overlaying the DB (#2463)", () => { + const manifest = parseFocusManifest({ settings: { autoCloseExemptLogins: ["Trusted-Regular", "another-one", "-bad", 42 as never] } }); + expect(manifest.settings.autoCloseExemptLogins).toEqual(["Trusted-Regular", "another-one"]); // invalid entries dropped + const eff = resolveEffectiveSettings({ autoCloseExemptLogins: ["db-only"] } as unknown as RepositorySettings, manifest); + expect(eff.autoCloseExemptLogins).toEqual(["Trusted-Regular", "another-one"]); // yml overlays (replaces) DB + // An empty/all-invalid block never blanks the DB-configured list (only set when a valid entry survives). + const noOverride = resolveEffectiveSettings({ autoCloseExemptLogins: ["keep-me"] } as unknown as RepositorySettings, parseFocusManifest({ settings: { autoCloseExemptLogins: ["-bad"] } })); + expect(noOverride.autoCloseExemptLogins).toEqual(["keep-me"]); + }); + it("an EXPLICIT yml null force-clears a DB-configured cap, distinct from an omitted key (regression, gate finding on #2467)", () => { // Omitted key preserves the DB value (already covered above); an explicit `null` must ALSO be able to // override a DB-configured cap back to "no cap" — the documented `yml > DB > null` precedence otherwise diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 2a6b37c983..ea4d616b84 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -11010,6 +11010,416 @@ describe("queue processors", () => { expect(JSON.stringify(usageEvents)).not.toMatch(/deliveryId|wallet|hotkey|raw trust/i); }); + describe("review-nag cooldown (#2463)", () => { + // Reusable stub covering everything the normal @gittensory Q&A dispatch needs (token, collaborator + // permission, comment GET/search + POST) PLUS the maintenance close path (label GET/POST, PR PATCH) — + // a superset so every scenario below (fall-through OR short-circuit) can share one fetch handler. + function stubReviewNagFetch(prNumber: number, seen: { comments: string[]; labels: string[]; closed: boolean }) { + 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("/collaborators/") && url.includes("/permission")) return Response.json({ permission: "none" }); + if (url.endsWith(`/pulls/${prNumber}`) && method === "PATCH") { + seen.closed = JSON.parse(String(init?.body ?? "{}")).state === "closed"; + return Response.json({ number: prNumber, state: "closed" }); + } + if (url.endsWith(`/pulls/${prNumber}`)) return Response.json({ number: prNumber, state: "open", head: { sha: `sha${prNumber}` }, mergeable_state: "clean" }); + if (url.includes(`/issues/${prNumber}/labels`) && method === "GET") return Response.json([]); + if (url.includes(`/issues/${prNumber}/labels`) && method === "POST") { + seen.labels.push(...((JSON.parse(String(init?.body ?? "{}")).labels ?? []) as string[])); + return Response.json([]); + } + // Repo-level label definition (createMissingLabel: true probes/creates the label before applying it). + if (url.endsWith("/labels") && method === "POST") return Response.json({ name: JSON.parse(String(init?.body ?? "{}")).name }, { status: 201 }); + if (url.includes(`/issues/${prNumber}/comments`) && method === "GET") return Response.json([]); + if (url.includes(`/issues/${prNumber}/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 }); + }); + } + + it("is off by default — no ping is tracked and no cooldown action fires", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 200, title: "Off by default", state: "open", user: { login: "chatty" }, author_association: "NONE", labels: [], body: "" }); + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubReviewNagFetch(200, seen); + await processJob(env, { + type: "github-webhook", + deliveryId: "nag-off-default", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 200, title: "Off by default", state: "open", pull_request: {}, user: { login: "chatty" }, author_association: "NONE" }, + comment: { id: 1, body: "@gittensory help", user: { login: "chatty", type: "User" }, author_association: "NONE" }, + }, + }); + const pings = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.review_nag_ping'").first<{ n: number }>(); + expect(pings?.n).toBe(0); + expect(seen.closed).toBe(false); + }); + + it("records pings under the configured threshold without acting; the normal @gittensory reply still proceeds", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "close", reviewNagMaxPings: 3 }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 201, title: "Under threshold", state: "open", user: { login: "chatty" }, author_association: "NONE", labels: [], body: "" }); + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubReviewNagFetch(201, seen); + await processJob(env, { + type: "github-webhook", + deliveryId: "nag-under-threshold", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 201, title: "Under threshold", state: "open", pull_request: {}, user: { login: "chatty" }, author_association: "NONE" }, + comment: { id: 1, body: "@gittensory help", user: { login: "chatty", type: "User" }, author_association: "NONE" }, + }, + }); + const pings = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.review_nag_ping'").first<{ n: number }>(); + expect(pings?.n).toBe(1); // the ping is recorded (1st of 3 allowed) + const applied = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.review_nag_cooldown_applied'").first<{ n: number }>(); + expect(applied?.n).toBe(0); // but no cooldown action — under threshold + expect(seen.closed).toBe(false); + // The review-nag hook returned false (fell through) — proven by the NORMAL mention-command dispatch + // making its own (here: unauthorized-skip) decision, rather than review-nag's short-circuit ever firing. + const skipped = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.agent_command_skipped'").first<{ n: number }>(); + expect(skipped?.n).toBeGreaterThanOrEqual(1); + }); + + it("hold policy: posts a cooldown reply and short-circuits once the threshold is crossed", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "hold", reviewNagMaxPings: 3, reviewNagCooldownDays: 5 }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 202, title: "Hold cooldown", state: "open", user: { login: "chatty" }, author_association: "NONE", labels: [], body: "" }); + for (let i = 0; i < 3; i += 1) { + await repositoriesModule.recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "JSONbored/gittensory#202", outcome: "completed" }); + } + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubReviewNagFetch(202, seen); + await processJob(env, { + type: "github-webhook", + deliveryId: "nag-hold", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 202, title: "Hold cooldown", state: "open", pull_request: {}, user: { login: "chatty" }, author_association: "NONE" }, + comment: { id: 4, body: "@gittensory help", user: { login: "chatty", type: "User" }, author_association: "NONE" }, + }, + }); + expect(seen.closed).toBe(false); + expect(seen.comments.some((c) => c.includes("cooldown limit"))).toBe(true); + // Only ONE comment posted — the short-circuit skipped the normal answer-card dispatch. + expect(seen.comments).toHaveLength(1); + const applied = await env.DB.prepare("select outcome, detail from audit_events where event_type = 'github_app.review_nag_cooldown_applied'").first<{ outcome: string; detail: string }>(); + expect(applied?.outcome).toBe("completed"); + expect(applied?.detail).toContain("hold applied"); + }); + + it("close policy on a PR thread: labels + closes once the threshold is crossed, with no merit review", async () => { + 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: ["issue_comment"] }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "close", reviewNagMaxPings: 3, autonomy: { close: "auto", label: "auto" } }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { reviewNagLabel: "too-chatty" } }, "repo_file"); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 203, title: "Close cooldown", state: "open", user: { login: "chatty" }, head: { sha: "sha203" }, author_association: "NONE", labels: [], body: "" }); + for (let i = 0; i < 3; i += 1) { + await repositoriesModule.recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "JSONbored/gittensory#203", outcome: "completed" }); + } + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubReviewNagFetch(203, seen); + await processJob(env, { + type: "github-webhook", + deliveryId: "nag-close", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 203, title: "Close cooldown", state: "open", pull_request: {}, user: { login: "chatty" }, author_association: "NONE" }, + comment: { id: 4, body: "@gittensory help", user: { login: "chatty", type: "User" }, author_association: "NONE" }, + }, + }); + expect(seen.closed).toBe(true); + expect(seen.labels).toContain("too-chatty"); // configurable label, not hardcoded + expect(seen.comments.some((c) => c.includes("chatty") && c.includes("4 times"))).toBe(true); + 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).toBeGreaterThanOrEqual(1); + }); + + it("close policy degrades to hold on an ISSUE thread (no closeIssue primitive yet)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "close", reviewNagMaxPings: 3 }); + for (let i = 0; i < 3; i += 1) { + await repositoriesModule.recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "JSONbored/gittensory#204", outcome: "completed" }); + } + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubReviewNagFetch(204, seen); + await processJob(env, { + type: "github-webhook", + deliveryId: "nag-issue-degrade", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 204, title: "Plain issue", state: "open", user: { login: "chatty" }, author_association: "NONE" }, + comment: { id: 4, body: "@gittensory help", user: { login: "chatty", type: "User" }, author_association: "NONE" }, + }, + }); + expect(seen.closed).toBe(false); // no closeIssue primitive — degrades to hold + expect(seen.comments.some((c) => c.includes("cooldown limit"))).toBe(true); + }); + + it("never throttles an exempt login, even over threshold", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "close", reviewNagMaxPings: 3, autoCloseExemptLogins: ["chatty"] }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 205, title: "Exempt author", state: "open", user: { login: "chatty" }, author_association: "NONE", labels: [], body: "" }); + for (let i = 0; i < 5; i += 1) { + await repositoriesModule.recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "JSONbored/gittensory#205", outcome: "completed" }); + } + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubReviewNagFetch(205, seen); + await processJob(env, { + type: "github-webhook", + deliveryId: "nag-exempt", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 205, title: "Exempt author", state: "open", pull_request: {}, user: { login: "chatty" }, author_association: "NONE" }, + comment: { id: 4, body: "@gittensory help", user: { login: "chatty", type: "User" }, author_association: "NONE" }, + }, + }); + expect(seen.closed).toBe(false); + const applied = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.review_nag_cooldown_applied'").first<{ n: number }>(); + expect(applied?.n).toBe(0); + }); + + it("never throttles a third party pinging on someone else's PR — only the thread's OWN author is tracked", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "close", reviewNagMaxPings: 3 }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 206, title: "Third party pinger", state: "open", user: { login: "pr-author" }, author_association: "NONE", labels: [], body: "" }); + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubReviewNagFetch(206, seen); + for (let i = 0; i < 5; i += 1) { + await processJob(env, { + type: "github-webhook", + deliveryId: `nag-third-party-${i}`, + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 206, title: "Third party pinger", state: "open", pull_request: {}, user: { login: "pr-author" }, author_association: "NONE" }, + comment: { id: i, body: "@gittensory help", user: { login: "bystander", type: "User" }, author_association: "NONE" }, + }, + }); + } + const pings = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.review_nag_ping'").first<{ n: number }>(); + expect(pings?.n).toBe(0); // never even tracked — the commenter is not the thread's own author + expect(seen.closed).toBe(false); + }); + + it("no-op owner-exemption when repoFullName has no slash (repoOwner is empty — never wrongly matches the commenter)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { + installation: { id: 123, account: { login: "", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["issue_comment"] }, + repositories: [{ name: "noslash", full_name: "noslash", private: false, owner: { login: "" } }], + }); + await upsertRepositorySettings(env, { repoFullName: "noslash", reviewNagPolicy: "hold", reviewNagMaxPings: 3 }); + for (let i = 0; i < 3; i += 1) { + await repositoriesModule.recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "noslash#209", outcome: "completed" }); + } + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubReviewNagFetch(209, seen); + await processJob(env, { + type: "github-webhook", + deliveryId: "nag-noslash", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "noslash", full_name: "noslash", private: false, owner: { login: "" } }, + issue: { number: 209, title: "Slash-free repo", state: "open", pull_request: {}, user: { login: "chatty" }, author_association: "NONE" }, + comment: { id: 4, body: "@gittensory help", user: { login: "chatty", type: "User" }, author_association: "NONE" }, + }, + }); + // repoOwner="" (branch false) → commenter "chatty" never equals "" → the owner-exemption is skipped and + // the throttle still engages normally (the comment post itself can't succeed for a slash-free repo — no + // owner/repo to target — but that failure is swallowed by design, same as every other best-effort notice + // in this file). Proven by reaching + completing the hold branch without the handler crashing. + const applied = await env.DB.prepare("select outcome from audit_events where event_type = 'github_app.review_nag_cooldown_applied'").first<{ outcome: string }>(); + expect(applied?.outcome).toBe("completed"); + }); + + it("never throttles the literal repo owner self-pinging their own PR", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "close", reviewNagMaxPings: 1 }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 207, title: "Owner PR", state: "open", user: { login: "JSONbored" }, author_association: "OWNER", labels: [], body: "" }); + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubReviewNagFetch(207, seen); + for (let i = 0; i < 3; i += 1) { + await processJob(env, { + type: "github-webhook", + deliveryId: `nag-owner-${i}`, + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 207, title: "Owner PR", state: "open", pull_request: {}, user: { login: "JSONbored" }, author_association: "OWNER" }, + comment: { id: i, body: "@gittensory help", user: { login: "JSONbored", type: "User" }, author_association: "OWNER" }, + }, + }); + } + const pings = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.review_nag_ping'").first<{ n: number }>(); + expect(pings?.n).toBe(0); + expect(seen.closed).toBe(false); + }); + + it("never throttles an ADMIN_GITHUB_LOGINS fleet-operator, even over threshold", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), ADMIN_GITHUB_LOGINS: "fleet-admin" }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "close", reviewNagMaxPings: 3 }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 208, title: "Admin PR", state: "open", user: { login: "fleet-admin" }, author_association: "NONE", labels: [], body: "" }); + for (let i = 0; i < 5; i += 1) { + await repositoriesModule.recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "fleet-admin", targetKey: "JSONbored/gittensory#208", outcome: "completed" }); + } + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubReviewNagFetch(208, seen); + await processJob(env, { + type: "github-webhook", + deliveryId: "nag-admin", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 208, title: "Admin PR", state: "open", pull_request: {}, user: { login: "fleet-admin" }, author_association: "NONE" }, + comment: { id: 6, body: "@gittensory help", user: { login: "fleet-admin", type: "User" }, author_association: "NONE" }, + }, + }); + expect(seen.closed).toBe(false); + const applied = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.review_nag_cooldown_applied'").first<{ n: number }>(); + expect(applied?.n).toBe(0); + }); + + it("hold policy respects agentDryRun — records a denied cooldown-applied audit and never posts the reply live (#2258 parity)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "hold", reviewNagMaxPings: 3, agentDryRun: true }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 209, title: "Dry-run hold", state: "open", user: { login: "chatty" }, author_association: "NONE", labels: [], body: "" }); + for (let i = 0; i < 3; i += 1) { + await repositoriesModule.recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "JSONbored/gittensory#209", outcome: "completed" }); + } + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubReviewNagFetch(209, seen); + await processJob(env, { + type: "github-webhook", + deliveryId: "nag-hold-dryrun", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 209, title: "Dry-run hold", state: "open", pull_request: {}, user: { login: "chatty" }, author_association: "NONE" }, + comment: { id: 4, body: "@gittensory help", user: { login: "chatty", type: "User" }, author_association: "NONE" }, + }, + }); + expect(seen.comments).toHaveLength(0); // dry-run — no live comment posted + const applied = await env.DB.prepare("select outcome from audit_events where event_type = 'github_app.review_nag_cooldown_applied'").first<{ outcome: string }>(); + expect(applied?.outcome).toBe("denied"); + }); + + it("close policy falls through harmlessly when the PR is no longer open by the time the threshold fires", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "close", reviewNagMaxPings: 3 }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 210, title: "Already closed", state: "closed", user: { login: "chatty" }, author_association: "NONE", labels: [], body: "" }); + for (let i = 0; i < 3; i += 1) { + await repositoriesModule.recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "JSONbored/gittensory#210", outcome: "completed" }); + } + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubReviewNagFetch(210, seen); + await processJob(env, { + type: "github-webhook", + deliveryId: "nag-already-closed", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 210, title: "Already closed", state: "closed", pull_request: {}, user: { login: "chatty" }, author_association: "NONE" }, + comment: { id: 4, body: "@gittensory help", user: { login: "chatty", type: "User" }, author_association: "NONE" }, + }, + }); + expect(seen.closed).toBe(false); + const applied = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.review_nag_cooldown_applied'").first<{ n: number }>(); + expect(applied?.n).toBe(0); // fell through silently — nothing left to act on + }); + + it("close policy records a denied cooldown-applied audit when autonomy is not acting for label/close (empty plan)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "close", reviewNagMaxPings: 3, autonomy: {} }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 211, title: "Observe-only autonomy", state: "open", user: { login: "chatty" }, head: { sha: "sha211" }, author_association: "NONE", labels: [], body: "" }); + for (let i = 0; i < 3; i += 1) { + await repositoriesModule.recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "JSONbored/gittensory#211", outcome: "completed" }); + } + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubReviewNagFetch(211, seen); + await processJob(env, { + type: "github-webhook", + deliveryId: "nag-observe-only", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 211, title: "Observe-only autonomy", state: "open", pull_request: {}, user: { login: "chatty" }, author_association: "NONE" }, + comment: { id: 4, body: "@gittensory help", user: { login: "chatty", type: "User" }, author_association: "NONE" }, + }, + }); + expect(seen.closed).toBe(false); + const applied = await env.DB.prepare("select outcome, detail from audit_events where event_type = 'github_app.review_nag_cooldown_applied'").first<{ outcome: string; detail: string }>(); + expect(applied?.outcome).toBe("denied"); + expect(applied?.detail).toContain("autonomy is not acting"); + }); + + it("close policy denies the mutation (never crashes) when no installation is on record — installationPermissions falls back to null", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "close", reviewNagMaxPings: 3, autonomy: { close: "auto", label: "auto" } }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 212, title: "No installation row", state: "open", user: { login: "chatty" }, head: { sha: "sha212" }, author_association: "NONE", labels: [], body: "" }); + for (let i = 0; i < 3; i += 1) { + await repositoriesModule.recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "JSONbored/gittensory#212", outcome: "completed" }); + } + const seen = { comments: [] as string[], labels: [] as string[], closed: false }; + stubReviewNagFetch(212, seen); + await processJob(env, { + type: "github-webhook", + deliveryId: "nag-no-installation", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 212, title: "No installation row", state: "open", pull_request: {}, user: { login: "chatty" }, author_association: "NONE" }, + comment: { id: 4, body: "@gittensory help", user: { login: "chatty", type: "User" }, author_association: "NONE" }, + }, + }); + expect(seen.closed).toBe(false); // no installation permissions on record — the write-permission gate denies it + const closeAudit = await env.DB.prepare("select outcome from audit_events where event_type = 'agent.action.close'").first<{ outcome: string }>(); + expect(closeAudit?.outcome).toBe("denied"); + }); + }); + it("denies a maintainer Q&A command from an org member without real repo permission (#788)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", {