diff --git a/apps/gittensory-ui/public/openapi.json b/apps/gittensory-ui/public/openapi.json index 7e8bf011c6..a94f4b8658 100644 --- a/apps/gittensory-ui/public/openapi.json +++ b/apps/gittensory-ui/public/openapi.json @@ -8955,6 +8955,16 @@ "type": "number", "nullable": true }, + "aiReviewLowConfidenceDisposition": { + "type": "string", + "nullable": true, + "enum": [ + "one_shot", + "hold_for_review", + "advisory_only", + null + ] + }, "aiReviewCombine": { "type": "string", "nullable": true, @@ -9383,6 +9393,9 @@ "agentDryRun": { "type": "boolean" }, + "agentGlobalFreezeOverride": { + "type": "boolean" + }, "contributorOpenPrCap": { "type": "integer", "nullable": true, @@ -9524,6 +9537,14 @@ "moderationBannedLabel": { "type": "string" }, + "skipAutomationBotAuthors": { + "type": "string", + "enum": [ + "inherit", + "off", + "enabled" + ] + }, "reviewEvasionProtection": { "type": "string", "enum": [ @@ -9571,9 +9592,6 @@ "advisory" ] }, - "message": { - "type": "string" - }, "requireViewports": { "type": "array", "items": { @@ -9586,6 +9604,9 @@ "type": "string" } }, + "message": { + "type": "string" + }, "skillFileUrl": { "type": "string" } @@ -9606,19 +9627,6 @@ "updatedAt": { "type": "string", "nullable": true - }, - "agentGlobalFreezeOverride": { - "type": "boolean" - }, - "aiReviewLowConfidenceDisposition": { - "type": "string", - "nullable": true, - "enum": [ - "one_shot", - "hold_for_review", - "advisory_only", - null - ] } }, "required": [ diff --git a/migrations/0143_repository_skip_automation_bot_authors.sql b/migrations/0143_repository_skip_automation_bot_authors.sql new file mode 100644 index 0000000000..b62685b31b --- /dev/null +++ b/migrations/0143_repository_skip_automation_bot_authors.sql @@ -0,0 +1,8 @@ +-- Waste elimination for known automation authors (github-actions[bot]/release-please, Renovate, +-- Dependabot -- settings/agent-actions.ts's PROTECTED_AUTOCLOSE_AUTHORS): skip AI review, gate evaluation, +-- and public-surface publish entirely for a PR genuinely triggered by one of these, not just suppress +-- output like review.auto_review.ignore_authors already does. 'inherit' (default) defers to the +-- GITTENSORY_SKIP_AUTOMATION_BOT_PRS global default (itself default-ON); 'off'/'enabled' fully override +-- the global default in either direction for this repo. Mirrors moderation_gate_mode's inherit/off/enabled +-- shape (0105). +ALTER TABLE repository_settings ADD COLUMN skip_automation_bot_authors TEXT NOT NULL DEFAULT 'inherit'; diff --git a/scripts/check-docs-drift.mjs b/scripts/check-docs-drift.mjs index 3bb86680cf..8fd310b243 100644 --- a/scripts/check-docs-drift.mjs +++ b/scripts/check-docs-drift.mjs @@ -90,7 +90,7 @@ export function extractRepositorySettingsFields(typesText) { } /** RepositorySettings fields deliberately excluded from the "every field must have SOME - * `.gittensory.yml.example` mention" check below, for two distinct reasons -- flagging either as "undocumented" + * `.gittensory.yml.example` mention" check below, for three distinct reasons -- flagging any as "undocumented" * would be a false drift signal, not a real gap: * - Not a maintainer-settable knob at all: `repoFullName` is the row's own identity key (set once at * creation, the opposite of something a maintainer overrides via config); `createdAt`/`updatedAt` are @@ -103,8 +103,19 @@ export function extractRepositorySettingsFields(typesText) { * exclusion, with the same rationale, in `SETTINGS_OPERATOR_ONLY_FIELDS` in * test/unit/focus-manifest.test.ts's `.gittensory.yml.example field-exhaustiveness` suite. (An #4617 audit * pass first flagged this field as an undocumented gap without that context; cross-checking the existing - * exhaustiveness suite before "fixing" it here caught the false positive.) */ -const NOT_YML_CONFIGURABLE_SETTINGS_FIELDS = new Set(["repoFullName", "createdAt", "updatedAt", "agentGlobalFreezeOverride"]); + * exhaustiveness suite before "fixing" it here caught the false positive.) + * - `skipAutomationBotAuthors`: genuinely settable (global env default + per-repo `inherit`/`off`/`enabled` + * override, mirroring `moderationGateMode`'s shape), but DELIBERATELY not wired into the + * FocusManifest/`.gittensory.yml` parsing path -- DB-only for now, confirmed as an intentional scope choice + * for this feature rather than an oversight. It is correctly absent from `FocusManifestSettings` (so the + * separate `.gittensory.yml.example` field-exhaustiveness suite never expected a token for it either). */ +const NOT_YML_CONFIGURABLE_SETTINGS_FIELDS = new Set([ + "repoFullName", + "createdAt", + "updatedAt", + "agentGlobalFreezeOverride", + "skipAutomationBotAuthors", +]); /** RepositorySettings fields whose `.gittensory.yml.example` documentation exists under a DIFFERENT, shorter * name than the field itself -- almost always because the yml groups several sibling fields under one named diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 80e687b920..9b1bb96e88 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -575,6 +575,7 @@ export async function getRepositorySettings(env: Env, fullName: string): Promise moderationRules: undefined, moderationWarningLabel: undefined, moderationBannedLabel: undefined, + skipAutomationBotAuthors: "inherit", reviewEvasionProtection: "close", // #4011: default-ON -- see normalizeReviewEvasionProtection's doc comment reviewEvasionLabel: DEFAULT_REVIEW_EVASION_LABEL, reviewEvasionComment: true, @@ -659,6 +660,7 @@ export async function getRepositorySettings(env: Env, fullName: string): Promise moderationRules: parseModerationRulesColumn(row.moderationRulesJson), moderationWarningLabel: normalizeModerationLabel(row.moderationWarningLabel), moderationBannedLabel: normalizeModerationLabel(row.moderationBannedLabel), + skipAutomationBotAuthors: normalizeSkipAutomationBotAuthors(row.skipAutomationBotAuthors), reviewEvasionProtection: normalizeReviewEvasionProtection(row.reviewEvasionProtection), reviewEvasionLabel: row.reviewEvasionLabel, reviewEvasionComment: row.reviewEvasionComment, @@ -777,6 +779,7 @@ export async function upsertRepositorySettings(env: Env, settings: Partial nowIso()), updatedAt: text("updated_at").notNull().$defaultFn(() => nowIso()), }); diff --git a/src/env.d.ts b/src/env.d.ts index 0d7a2c8661..2b23e498b1 100644 --- a/src/env.d.ts +++ b/src/env.d.ts @@ -462,6 +462,12 @@ declare global { * flagged (see the same-author guard in buildCollisionReport). Default OFF — unset/false leaves every * PullRequestRecord's changedFiles unset, byte-identical to today. See src/signals/engine.ts prItem. */ GITTENSORY_OPEN_PR_FILE_COLLISION?: string; + /** Waste elimination for known automation authors (settings/automation-bot-skip.ts): skip AI review, gate + * evaluation, and public-surface publish entirely for a PR/event genuinely triggered by release-please's + * github-actions[bot], Renovate, or Dependabot. Default-ON, unlike most flags above — see that module's + * own doc comment for why. A repo can override via its own repository_settings.skip_automation_bot_authors + * column ("off"/"enabled"), independent of this global default. */ + GITTENSORY_SKIP_AUTOMATION_BOT_PRS?: string; /** D1 size/row-count observability probe (#3810): the Cloudflare account id that owns the D1 database to * monitor. Presence of this AND the two vars below IS the enablement switch (see isD1SizeProbeEnabled, * src/selfhost/d1-size-probe.ts) -- unset/blank ⇒ the probe never runs, byte-identical to today. Most diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index 334292f216..98c0459b53 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -817,6 +817,7 @@ export const RepositorySettingsSchema = z moderationRules: z.array(z.enum(["contributor_cap", "blacklist", "review_nag", "review_evasion"])).optional(), moderationWarningLabel: z.string().optional(), moderationBannedLabel: z.string().optional(), + skipAutomationBotAuthors: z.enum(["inherit", "off", "enabled"]).optional(), reviewEvasionProtection: z.enum(["off", "close"]).optional(), reviewEvasionLabel: z.string().nullable().optional(), reviewEvasionComment: z.boolean().optional(), diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 8e25c88d93..85ae74259b 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -307,6 +307,12 @@ import { type PlannedAgentAction, } from "../settings/agent-actions"; import { isAutoCloseExempt } from "../settings/auto-close-exempt"; +import { + isSkipAutomationBotPullRequestsEnabledGlobally, + isTrustedAutomationBotAuthor, + isTrustedAutomationBotWebhookActor, + resolveSkipAutomationBotPullRequests, +} from "../settings/automation-bot-skip"; import { resolveGlobalContributorOpenItemCap, resolveGlobalContributorOpenItemCapForMiner } from "../settings/global-contributor-cap"; import { detectMigrationCollisions, extractMigrationNumber, KNOWN_MIGRATION_DUPLICATES } from "../db/migration-collisions"; import { listMigrationFilenamesAtRef } from "../github/migration-tree"; @@ -3669,6 +3675,17 @@ async function reReviewStoredPullRequest( ]); let pr = await getPullRequest(env, repoFullName, prNumber); if (!pr || pr.state !== "open") return; + // Waste elimination for known automation authors (settings/automation-bot-skip.ts): every re-entry path + // that can reach a PR without a fresh webhook (a scheduled sweep, CI-completion, or linked-issue-change + // re-review -- all funnel through this function via regatePullRequest) funnels through here, so this one + // check closes all of them. Uses the STORED author (isTrustedAutomationBotAuthor), not a live webhook + // sender -- see that function's own doc comment for why that's still safe: authorLogin is immutable, + // GitHub-attested metadata already verified against the actor at the original `opened` webhook. + if ( + resolveSkipAutomationBotPullRequests(isSkipAutomationBotPullRequestsEnabledGlobally(env), settings.skipAutomationBotAuthors) && + isTrustedAutomationBotAuthor(pr.authorLogin) + ) + return; const autoreviewPaused = await hasAutoreviewPausedMarker(env, repoFullName, prNumber); const liveFacts = createLiveGithubFacts(); // #sweep-resync: RESYNC the stored PR to its LIVE head before reviewing. The self-host relay can drop the @@ -6321,6 +6338,47 @@ async function handlePullRequestWebhookEvent( // Resolve settings first so the self-authored + open-reference live-fetch fallbacks only fire when their // respective gates are in block mode. const settings = await resolveRepositorySettings(env, repoFullName); + // Waste elimination for known automation authors (settings/automation-bot-skip.ts): a PR/event genuinely + // triggered by release-please's github-actions[bot], Renovate, or Dependabot never needs AI review, gate + // evaluation, or a public-surface publish. Checked here (not earlier) because it needs `settings` for the + // per-repo override, but BEFORE the expensive Promise.all/refreshPullRequestDetails/AI/gate work below -- + // isTrustedAutomationBotWebhookActor is the security-critical check (see its own doc comment): it verifies + // the ACTOR WHO TRIGGERED THIS EVENT, not just the PR's stored author, so a human pushing to an existing + // bot PR's branch still gets full review of their own commits. + if ( + resolveSkipAutomationBotPullRequests(isSkipAutomationBotPullRequestsEnabledGlobally(env), settings.skipAutomationBotAuthors) && + isTrustedAutomationBotWebhookActor(payload.sender, pr.authorLogin) + ) { + await recordAuditEvent(env, { + eventType: "github_app.automation_bot_pr_skipped", + actor: payload.sender?.login ?? pr.authorLogin, + targetKey: `${repoFullName}#${pr.number}`, + outcome: "completed", + detail: "skipped: known automation-bot author (release-please/Renovate/Dependabot)", + metadata: { deliveryId, repoFullName, eventName, action: payload.action ?? null }, + }).catch((error) => { + /* v8 ignore next -- best-effort: audit recording never blocks (or un-skips) the webhook. */ + console.warn( + JSON.stringify({ + level: "warn", + event: "automation_bot_pr_skip_audit_failed", + deliveryId, + repository: repoFullName, + error: errorMessage(error), + }), + ); + }); + await recordWebhookEvent(env, { + deliveryId, + eventName, + action: payload.action, + installationId: payload.installation?.id, + repositoryFullName: payload.repository?.full_name, + payloadHash: "processed", + status: "processed", + }); + return true; + } const [repo, cachedOtherOpenPullRequests, { linkedIssueAuthorLogins, confirmedNoOpenLinkedIssue }] = await Promise.all([ getRepository(env, repoFullName), diff --git a/src/settings/automation-bot-skip.ts b/src/settings/automation-bot-skip.ts new file mode 100644 index 0000000000..8aa0a9519c --- /dev/null +++ b/src/settings/automation-bot-skip.ts @@ -0,0 +1,74 @@ +import { isProtectedAutomationAuthor } from "./agent-actions"; + +export type AutomationBotSkipMode = "inherit" | "off" | "enabled"; + +/** Truthy convention matches the rest of this codebase (`/^(1|true|yes|on)$/i`, e.g. isReputationEnabled), + * inverted: this flag defaults ON (skip), so only an explicit falsy value ("0"/"false"/"no"/"off") turns it + * off install-wide. Unlike most `GITTENSORY_REVIEW_*` flags (opt-in, default off), eliminating AI/gate spend + * on PRs from known, maintainer-owned automation (release-please's github-actions[bot], Renovate, + * Dependabot) is safe and low-risk enough to be the sensible default -- it should not require every + * self-host operator to discover and separately opt into this. */ +export function isSkipAutomationBotPullRequestsEnabledGlobally(env: { GITTENSORY_SKIP_AUTOMATION_BOT_PRS?: string | undefined }): boolean { + return !/^(0|false|no|off)$/i.test((env.GITTENSORY_SKIP_AUTOMATION_BOT_PRS ?? "").trim()); +} + +/** Per-repo override resolved against the global default. Mirrors `ModerationGateMode`'s inherit/off/enabled + * shape (settings/moderation-rules.ts) but is symmetric -- "off" and "enabled" both fully override the + * global default in either direction, unlike moderation's global-is-authoritative asymmetry -- because this + * is a narrower, lower-stakes waste-reduction toggle (skip review for known automation), not a fleet-wide + * safety kill-switch, so there's no reason a repo opting IN should still be blocked by a globally-off + * default. */ +export function resolveSkipAutomationBotPullRequests(globalDefault: boolean, mode: AutomationBotSkipMode | null | undefined): boolean { + if (mode === "off") return false; + if (mode === "enabled") return true; + return globalDefault; +} + +/** + * SECURITY (do not weaken without re-reading this comment): decides whether the review pipeline may treat + * the CURRENT webhook event as bot-originated automation and skip full review for it. This is a trust + * boundary, not a convenience check -- getting it wrong lets a contributor slip a PR past review entirely. + * + * Checks the actor who triggered THIS SPECIFIC event (`sender`), never just the PR's original/stored author. + * A `pull_request` webhook's `sender` is "whoever performed the action that fired this event" -- for + * `opened`, that's whoever opened the PR; for `synchronize`, that's whoever pushed the new commits, which is + * NOT necessarily the PR's original author. If this checked only the stored PR author, an actor with write + * access to an EXISTING bot-authored PR's branch (a fork with "allow edits by maintainers" enabled, or a + * misconfigured branch permission) could push malicious commits onto that branch and inherit the bot's + * skip-review treatment for a `synchronize` event `sender` did not actually originate from the bot. + * + * Requires BOTH `sender` (this event's actor) AND the PR's own recorded author to be in the trusted set -- + * defense in depth: a legitimate bot-originated event always satisfies both (the bot both opened the PR and + * is the one pushing to it), so requiring both closes any path where they could diverge without narrowing + * the legitimate case. + * + * `sender.login`/`sender.type` are GitHub's own attestation of who/what performed the action, delivered in an + * HMAC-signed webhook payload verified before this ever runs (see github/webhook.ts) -- neither is spoofable + * by a contributor's own request. GitHub also does not permit a regular ("User"-type) account to register a + * `[bot]`-suffixed login, and each bot's login (e.g. "renovate[bot]") is tied to a single, globally-unique + * GitHub App slug no other party can claim -- so `isProtectedAutomationAuthor`'s exact-match allowlist + * (settings/agent-actions.ts) cannot be satisfied by an untrusted contributor's own account, and the + * `type === "Bot"` check is still required as defense in depth against a future looser login match. + */ +export function isTrustedAutomationBotWebhookActor( + sender: { login?: string | null | undefined; type?: string | null | undefined } | null | undefined, + prAuthorLogin: string | null | undefined, +): boolean { + return ( + sender?.type === "Bot" && + isProtectedAutomationAuthor(sender.login) && + isProtectedAutomationAuthor(prAuthorLogin) + ); +} + +/** + * Re-entry paths (a scheduled sweep, CI-completion, or linked-issue-change re-review -- all funnel through + * `reReviewStoredPullRequest`) have no live webhook `sender` to check; they're re-evaluating an ALREADY + * PERSISTED PR record, not processing a fresh, potentially actor-ambiguous event. The PR's stored author was + * already verified against `isTrustedAutomationBotWebhookActor` (both `sender` AND author) at the ORIGINAL + * `opened` webhook that created the row, and `authorLogin` is immutable GitHub-attested metadata (never + * user-editable) -- re-checking just the stored author here is safe and consistent with that original check. + */ +export function isTrustedAutomationBotAuthor(prAuthorLogin: string | null | undefined): boolean { + return isProtectedAutomationAuthor(prAuthorLogin); +} diff --git a/src/types.ts b/src/types.ts index c97bcac689..6b5979d63a 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1104,6 +1104,15 @@ export type RepositorySettings = { /** Moderation-rules engine: per-repo override of the label applied at >= the ban threshold. `undefined` ⇒ * the global config's `bannedLabel` (itself defaulting to `"mod:banned"`). */ moderationBannedLabel?: string | undefined; + /** Waste elimination for known automation authors (release-please's github-actions[bot], Renovate, + * Dependabot -- settings/agent-actions.ts's PROTECTED_AUTOCLOSE_AUTHORS): skip AI review, gate evaluation, + * and public-surface publish entirely for a PR/event genuinely triggered by one of these -- not just + * suppress output like {@link "./review-eligibility".ignoreAuthors}. `"inherit"` (the DB default) defers + * to the `GITTENSORY_SKIP_AUTOMATION_BOT_PRS` global default (itself default-ON, unlike most feature + * flags -- see settings/automation-bot-skip.ts's own doc comment for why); `"off"`/`"enabled"` fully + * override the global default in either direction for this repo. Always populated by the DB layer; + * optional so existing settings fixtures/callers need not be touched. */ + skipAutomationBotAuthors?: "inherit" | "off" | "enabled" | undefined; /** Review-evasion protection (#review-evasion-protection): a contributor closing or converting their OWN * PR to draft while gittensory has an ACTIVE review pass running against it is dodging the one-shot * review process. `"off"` (the default) disables detection entirely; `"close"` reopens (if needed) and diff --git a/test/helpers/d1.ts b/test/helpers/d1.ts index 4281d880e1..6c98bd7874 100644 --- a/test/helpers/d1.ts +++ b/test/helpers/d1.ts @@ -128,6 +128,9 @@ export function createTestEnv(overrides: Partial = {}): Env { // Per-repo review allowlist: default to the test repos so flag-ON wiring tests activate the // gated review features. Override to "" to assert the dormant (no-repo) default. GITTENSORY_REVIEW_REPOS: "JSONbored/gittensory,acme/widgets", + // Default-ON in production (settings/automation-bot-skip.ts); most tests don't involve a bot actor at + // all, so this default doesn't change their behavior. Tests exercising this feature override it directly. + GITTENSORY_SKIP_AUTOMATION_BOT_PRS: "true", ...overrides, }; } diff --git a/test/unit/automation-bot-skip.test.ts b/test/unit/automation-bot-skip.test.ts new file mode 100644 index 0000000000..65e78274ea --- /dev/null +++ b/test/unit/automation-bot-skip.test.ts @@ -0,0 +1,111 @@ +import { describe, expect, it } from "vitest"; +import { + isSkipAutomationBotPullRequestsEnabledGlobally, + isTrustedAutomationBotAuthor, + isTrustedAutomationBotWebhookActor, + resolveSkipAutomationBotPullRequests, +} from "../../src/settings/automation-bot-skip"; + +describe("isSkipAutomationBotPullRequestsEnabledGlobally", () => { + it("defaults ON when unset (unlike most GITTENSORY_REVIEW_* flags)", () => { + expect(isSkipAutomationBotPullRequestsEnabledGlobally({})).toBe(true); + expect(isSkipAutomationBotPullRequestsEnabledGlobally({ GITTENSORY_SKIP_AUTOMATION_BOT_PRS: undefined })).toBe(true); + expect(isSkipAutomationBotPullRequestsEnabledGlobally({ GITTENSORY_SKIP_AUTOMATION_BOT_PRS: "" })).toBe(true); + }); + + it("stays ON for an explicit truthy value", () => { + expect(isSkipAutomationBotPullRequestsEnabledGlobally({ GITTENSORY_SKIP_AUTOMATION_BOT_PRS: "true" })).toBe(true); + expect(isSkipAutomationBotPullRequestsEnabledGlobally({ GITTENSORY_SKIP_AUTOMATION_BOT_PRS: "1" })).toBe(true); + }); + + it("turns OFF only for an explicit falsy value, case-insensitively", () => { + for (const value of ["0", "false", "False", "FALSE", "no", "No", "off", "OFF"]) { + expect(isSkipAutomationBotPullRequestsEnabledGlobally({ GITTENSORY_SKIP_AUTOMATION_BOT_PRS: value })).toBe(false); + } + }); + + it("stays ON for whitespace around a truthy/garbage value", () => { + expect(isSkipAutomationBotPullRequestsEnabledGlobally({ GITTENSORY_SKIP_AUTOMATION_BOT_PRS: " false " })).toBe(false); + expect(isSkipAutomationBotPullRequestsEnabledGlobally({ GITTENSORY_SKIP_AUTOMATION_BOT_PRS: "banana" })).toBe(true); + }); +}); + +describe("resolveSkipAutomationBotPullRequests", () => { + it("inherit defers to the global default in both directions", () => { + expect(resolveSkipAutomationBotPullRequests(true, "inherit")).toBe(true); + expect(resolveSkipAutomationBotPullRequests(false, "inherit")).toBe(false); + }); + + it("null/undefined mode behaves the same as inherit", () => { + expect(resolveSkipAutomationBotPullRequests(true, null)).toBe(true); + expect(resolveSkipAutomationBotPullRequests(false, undefined)).toBe(false); + }); + + it("off fully overrides a globally-ON default", () => { + expect(resolveSkipAutomationBotPullRequests(true, "off")).toBe(false); + }); + + it("enabled fully overrides a globally-OFF default (symmetric, unlike moderation's global-authoritative gate)", () => { + expect(resolveSkipAutomationBotPullRequests(false, "enabled")).toBe(true); + }); +}); + +describe("isTrustedAutomationBotAuthor (re-entry paths: stored author only)", () => { + it("true for every known automation login, case-insensitively", () => { + expect(isTrustedAutomationBotAuthor("github-actions[bot]")).toBe(true); + expect(isTrustedAutomationBotAuthor("Renovate[Bot]")).toBe(true); + expect(isTrustedAutomationBotAuthor("dependabot[bot]")).toBe(true); + }); + + it("false for a human contributor, including a look-alike login", () => { + expect(isTrustedAutomationBotAuthor("JSONbored")).toBe(false); + expect(isTrustedAutomationBotAuthor("renovate")).toBe(false); // missing the [bot] suffix + expect(isTrustedAutomationBotAuthor(null)).toBe(false); + expect(isTrustedAutomationBotAuthor(undefined)).toBe(false); + }); +}); + +// SECURITY: these pin the exploit-resistance guarantee described in isTrustedAutomationBotWebhookActor's own +// doc comment. Do not relax any of the "false" cases below without re-reading that comment first. +describe("isTrustedAutomationBotWebhookActor (SECURITY: the live webhook actor, not just the PR author)", () => { + it("true for a genuine bot-originated event: sender IS the bot, type is Bot, and it's also the stored PR author", () => { + expect( + isTrustedAutomationBotWebhookActor({ login: "github-actions[bot]", type: "Bot" }, "github-actions[bot]"), + ).toBe(true); + expect(isTrustedAutomationBotWebhookActor({ login: "renovate[bot]", type: "Bot" }, "renovate[bot]")).toBe(true); + expect(isTrustedAutomationBotWebhookActor({ login: "dependabot[bot]", type: "Bot" }, "dependabot[bot]")).toBe(true); + }); + + it("true regardless of login casing (mirrors isProtectedAutomationAuthor's own case-insensitivity)", () => { + expect(isTrustedAutomationBotWebhookActor({ login: "RENOVATE[BOT]", type: "Bot" }, "Renovate[Bot]")).toBe(true); + }); + + it("EXPLOIT CASE: a human who gained push access to an existing bot PR's branch must NOT inherit the skip -- sender is the human triggering THIS event, even though the PR's original/stored author is still the bot", () => { + expect( + isTrustedAutomationBotWebhookActor({ login: "malicious-contributor", type: "User" }, "renovate[bot]"), + ).toBe(false); + }); + + it("false when sender's login matches but type does not say Bot (defense in depth against a future looser login match)", () => { + expect(isTrustedAutomationBotWebhookActor({ login: "renovate[bot]", type: "User" }, "renovate[bot]")).toBe(false); + }); + + it("false when sender is a genuine bot but NOT one of the trusted three (an untrusted third-party App/bot)", () => { + expect(isTrustedAutomationBotWebhookActor({ login: "some-other-app[bot]", type: "Bot" }, "some-other-app[bot]")).toBe(false); + }); + + it("false when the stored PR author does not ALSO match, even if sender does (defense in depth: both must agree)", () => { + expect(isTrustedAutomationBotWebhookActor({ login: "renovate[bot]", type: "Bot" }, "some-human-contributor")).toBe(false); + }); + + it("false (fail-safe) when sender is missing entirely", () => { + expect(isTrustedAutomationBotWebhookActor(null, "renovate[bot]")).toBe(false); + expect(isTrustedAutomationBotWebhookActor(undefined, "renovate[bot]")).toBe(false); + expect(isTrustedAutomationBotWebhookActor({}, "renovate[bot]")).toBe(false); + }); + + it("false when the PR author is missing entirely, even with a genuine bot sender", () => { + expect(isTrustedAutomationBotWebhookActor({ login: "renovate[bot]", type: "Bot" }, null)).toBe(false); + expect(isTrustedAutomationBotWebhookActor({ login: "renovate[bot]", type: "Bot" }, undefined)).toBe(false); + }); +}); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index f5654cae01..08ee66a714 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -6403,8 +6403,11 @@ describe("queue processors", () => { AI_PUBLIC_COMMENTS_ENABLED: "true", AI_DAILY_NEURON_BUDGET: "100000", }); - await seedRegateChurnRepo(env, { publicSurface: "comment_only" }); // #one-shot-review-cadence: isolate this test to the automation-bot-exemption-from-the-LABEL-freeze mechanism. + // #automation-bot-skip: ALSO isolate from the newer, broader automation-bot-skip.ts early-return in + // reReviewStoredPullRequest -- that skip would otherwise short-circuit before ever reaching the freeze + // logic this test targets, so it's explicitly turned off here too. + await seedRegateChurnRepo(env, { publicSurface: "comment_only", skipAutomationBotAuthors: "off" }); await upsertRepoFocusManifest(env, "JSONbored/gittensory", { review: { auto_review: { cadence: "continuous" } } }); await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 81, title: "Bot's held PR", state: "open", user: { login: "dependabot[bot]" }, head: { sha: "a81-v1" }, labels: [{ name: "manual-review" }], body: "Closes #1" }); await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 81, status: "complete", reviewsSyncedAt: new Date().toISOString() }); @@ -33459,3 +33462,132 @@ describe("auto-action convergence: end-to-end plan+execute for the general heuri expect(closeAudit?.n).toBe(0); }); }); + +// #automation-bot-skip: waste elimination for known automation authors (release-please's github-actions[bot], +// Renovate, Dependabot). End-to-end wiring on top of automation-bot-skip.test.ts's pure-function coverage -- +// these pin the webhook + re-entry integration points, including the SECURITY property that a human pushing +// to an existing bot PR's branch still gets full review of their own commits. +describe("automation-bot-skip: end-to-end webhook + re-entry wiring (#automation-bot-skip)", () => { + const basePayload = { + installation: { id: 9101, account: { login: "owner", id: 1, type: "Organization" } }, + repository: { name: "bot-skip-repo", full_name: "owner/bot-skip-repo", private: false, owner: { login: "owner" } }, + }; + + // resolveRepositorySettings itself probes for a config-as-code override (.gittensory.yml/.json in both the + // repo root and .github/) BEFORE the skip check can even run (it needs the resolved settings for the + // per-repo override) -- so those 4 raw.githubusercontent.com probes are unavoidable, pre-existing overhead + // on EVERY webhook, not the "waste" this feature eliminates. The real signal is that NOTHING beyond that + // touches the actual GitHub REST API (api.github.com) -- no installation-token fetch, no PR/files read, no + // comment/check-run publish, no AI provider call. + async function fetchCallTracker() { + const state = { urls: [] as string[] }; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + state.urls.push(input.toString()); + return new Response("not found", { status: 404 }); + }); + return state; + } + + it("a genuine bot-triggered PR (sender IS the bot, matching the stored author) is skipped entirely: audited, zero GitHub/AI fetch calls, delivery marked processed", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + const calls = await fetchCallTracker(); + + await processJob(env, { + type: "github-webhook", + deliveryId: "bot-skip-genuine", + eventName: "pull_request", + payload: { + action: "opened", + ...basePayload, + sender: { login: "renovate[bot]", type: "Bot" }, + pull_request: { number: 401, title: "chore(deps): bump foo", state: "open", user: { login: "renovate[bot]", type: "Bot" }, labels: [], body: "" }, + }, + }); + + expect(calls.urls.some((url) => url.includes("api.github.com"))).toBe(false); + const skipAudit = await env.DB.prepare("select detail, actor from audit_events where event_type = 'github_app.automation_bot_pr_skipped' and target_key = 'owner/bot-skip-repo#401'").first<{ detail: string; actor: string }>(); + expect(skipAudit?.actor).toBe("renovate[bot]"); + expect(skipAudit?.detail).toContain("automation-bot author"); + const webhookEvent = await env.DB.prepare("select status from webhook_events where delivery_id = 'bot-skip-genuine'").first<{ status: string }>(); + expect(webhookEvent?.status).toBe("processed"); + }); + + it("SECURITY: a human who pushes to an existing bot-authored PR's branch (synchronize) is NOT skipped -- the live webhook actor, not the stored author, gates the skip", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await fetchCallTracker(); + + await processJob(env, { + type: "github-webhook", + deliveryId: "bot-skip-exploit-attempt", + eventName: "pull_request", + payload: { + action: "synchronize", + ...basePayload, + sender: { login: "malicious-contributor", type: "User" }, + pull_request: { number: 402, title: "chore(deps): bump foo", state: "open", user: { login: "renovate[bot]", type: "Bot" }, labels: [], body: "", head: { sha: "hijacked-sha" } }, + }, + }); + + const skipAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.automation_bot_pr_skipped' and target_key = 'owner/bot-skip-repo#402'").first<{ n: number }>(); + expect(skipAudit?.n).toBe(0); + }); + + it("a per-repo 'off' override forces full review even for a genuine bot-triggered PR", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await fetchCallTracker(); + await upsertRepositorySettings(env, { repoFullName: "owner/bot-skip-repo", skipAutomationBotAuthors: "off" }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "bot-skip-repo-off-override", + eventName: "pull_request", + payload: { + action: "opened", + ...basePayload, + sender: { login: "dependabot[bot]", type: "Bot" }, + pull_request: { number: 403, title: "chore(deps): bump bar", state: "open", user: { login: "dependabot[bot]", type: "Bot" }, labels: [], body: "" }, + }, + }); + + const skipAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.automation_bot_pr_skipped' and target_key = 'owner/bot-skip-repo#403'").first<{ n: number }>(); + expect(skipAudit?.n).toBe(0); + }); + + it("a per-repo 'enabled' override skips a genuine bot-triggered PR even when the global default is OFF", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_SKIP_AUTOMATION_BOT_PRS: "false" }); + const calls = await fetchCallTracker(); + await upsertRepositorySettings(env, { repoFullName: "owner/bot-skip-repo", skipAutomationBotAuthors: "enabled" }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "bot-skip-repo-enabled-override", + eventName: "pull_request", + payload: { + action: "opened", + ...basePayload, + sender: { login: "github-actions[bot]", type: "Bot" }, + pull_request: { number: 404, title: "chore(release): 1.2.3", state: "open", user: { login: "github-actions[bot]", type: "Bot" }, labels: [], body: "" }, + }, + }); + + expect(calls.urls.some((url) => url.includes("api.github.com"))).toBe(false); + const skipAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'github_app.automation_bot_pr_skipped' and target_key = 'owner/bot-skip-repo#404'").first<{ n: number }>(); + expect(skipAudit?.n).toBe(1); + }); + + it("the re-entry sweep path (agent-regate-pr) also respects the skip for a stored bot author, without even the live resync fetch", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { action: "created", installation: { id: 9101, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: {}, events: [] } }); + await upsertRepositoryFromGitHub(env, { name: "bot-skip-repo", full_name: "owner/bot-skip-repo", private: false, owner: { login: "owner" } }, 9101); + await upsertPullRequestFromGitHub(env, "owner/bot-skip-repo", { number: 405, title: "chore(deps): bump baz", state: "open", user: { login: "renovate[bot]", type: "Bot" }, head: { sha: "sha405" }, labels: [], body: "" }); + const calls = await fetchCallTracker(); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "bot-skip-sweep", repoFullName: "owner/bot-skip-repo", prNumber: 405, installationId: 9101 }); + + // The re-entry check runs BEFORE even the live-head resync GET, so a genuine bot author skips without any + // GitHub REST API call at all -- not merely without a comment/check-run publish. + expect(calls.urls.some((url) => url.includes("api.github.com"))).toBe(false); + const stored = await getPullRequest(env, "owner/bot-skip-repo", 405); + expect(stored?.headSha).toBe("sha405"); + }); +}); diff --git a/test/unit/repository-settings-skip-automation-bot-authors.test.ts b/test/unit/repository-settings-skip-automation-bot-authors.test.ts new file mode 100644 index 0000000000..8aa18cc800 --- /dev/null +++ b/test/unit/repository-settings-skip-automation-bot-authors.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it } from "vitest"; +import { getRepositorySettings, upsertRepositorySettings } from "../../src/db/repositories"; +import { createTestEnv } from "../helpers/d1"; + +// #automation-bot-skip: skipAutomationBotAuthors ("inherit" | "off" | "enabled") mirrors moderationGateMode's +// shape (migrations/0105) -- see repository-settings-merge-train-mode.test.ts's own comment for the exact +// INSERT/UPDATE persistence bug this pattern guards against. +describe("repository_settings: skipAutomationBotAuthors persistence (#automation-bot-skip)", () => { + it("getRepositorySettings returns inherit for a repo with no DB row at all (defers to the global default)", async () => { + const env = createTestEnv(); + const settings = await getRepositorySettings(env, "acme/brand-new-repo"); + expect(settings.skipAutomationBotAuthors).toBe("inherit"); + }); + + it("an explicit skipAutomationBotAuthors persists on the FIRST upsert (INSERT path)", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/fresh-insert", skipAutomationBotAuthors: "enabled" }); + const settings = await getRepositorySettings(env, "acme/fresh-insert"); + expect(settings.skipAutomationBotAuthors).toBe("enabled"); + }); + + it("an explicit skipAutomationBotAuthors persists on a SECOND upsert of an already-existing row (UPDATE path)", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/existing-row" }); + const before = await getRepositorySettings(env, "acme/existing-row"); + expect(before.skipAutomationBotAuthors).toBe("inherit"); + + await upsertRepositorySettings(env, { repoFullName: "acme/existing-row", skipAutomationBotAuthors: "off" }); + const after = await getRepositorySettings(env, "acme/existing-row"); + expect(after.skipAutomationBotAuthors).toBe("off"); + }); + + it("a true read-modify-write caller (spread current settings, then re-upsert) carries skipAutomationBotAuthors forward explicitly", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/round-trip", skipAutomationBotAuthors: "enabled" }); + const settings = await getRepositorySettings(env, "acme/round-trip"); + expect(settings.skipAutomationBotAuthors).toBe("enabled"); + await upsertRepositorySettings(env, { ...settings, repoFullName: "acme/round-trip" }); + const after = await getRepositorySettings(env, "acme/round-trip"); + expect(after.skipAutomationBotAuthors).toBe("enabled"); + }); + + it("an invalid persisted DB value fails closed to inherit on read", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/malformed" }); + await env.DB.prepare("UPDATE repository_settings SET skip_automation_bot_authors = ? WHERE repo_full_name = ?").bind("sometimes", "acme/malformed").run(); + const settings = await getRepositorySettings(env, "acme/malformed"); + expect(settings.skipAutomationBotAuthors).toBe("inherit"); + }); +}); diff --git a/worker-configuration.d.ts b/worker-configuration.d.ts index 2e3a28fc87..ff5da0d2e0 100644 --- a/worker-configuration.d.ts +++ b/worker-configuration.d.ts @@ -1,5 +1,5 @@ /* eslint-disable */ -// Generated by Wrangler by running `wrangler types` (hash: 7cd863ec781ed395e3f5eb00399370bc) +// Generated by Wrangler by running `wrangler types` (hash: 31e2a5aa781186e51d85d560f8a9f25b) // Runtime types generated with workerd@1.20260701.1 2026-05-28 nodejs_compat interface __BaseEnv_Env { DB: D1Database; @@ -45,6 +45,7 @@ interface __BaseEnv_Env { GITTENSORY_PUBLIC_STATS_REPOS: "JSONbored/gittensory,JSONbored/awesome-claude,JSONbored/metagraphed"; GITTENSORY_DUPLICATE_WINNER: "true"; GITTENSORY_OPEN_PR_FILE_COLLISION: "true"; + GITTENSORY_SKIP_AUTOMATION_BOT_PRS: "true"; RATE_LIMITER: DurableObjectNamespace; } declare namespace Cloudflare { @@ -97,6 +98,7 @@ declare namespace NodeJS { | "GITTENSORY_REVIEW_SCREENSHOTS" | "GITTENSORY_REVIEW_SELFTUNE" | "GITTENSORY_REVIEW_UNIFIED_COMMENT" + | "GITTENSORY_SKIP_AUTOMATION_BOT_PRS" | "GITTENSORY_SWEEP_WATCHDOG" | "PUBLIC_API_ORIGIN" | "PUBLIC_SITE_ORIGIN" diff --git a/wrangler.jsonc b/wrangler.jsonc index 23ea84e76c..80430c3e85 100644 --- a/wrangler.jsonc +++ b/wrangler.jsonc @@ -201,6 +201,12 @@ // sharesMeaningfulFile guard in buildCollisionReport). Re-validated post-fix: every remaining flagged pair is // backed by a real shared file. "GITTENSORY_OPEN_PR_FILE_COLLISION": "true", + // Waste elimination for known automation authors (settings/automation-bot-skip.ts): skip AI review, gate + // evaluation, and public-surface publish entirely for a PR/event genuinely triggered by github-actions[bot] + // (release-please), Renovate, or Dependabot. Default-ON, unlike most flags above -- see that module's own + // doc comment for why. A repo can override via its own repository_settings.skip_automation_bot_authors + // column ("off"/"enabled"), independent of this global default. + "GITTENSORY_SKIP_AUTOMATION_BOT_PRS": "true", }, "routes": [ {