Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gittensory.yml.example
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ settings:
# 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.
# reviewNagCooldownDays: 5 # Positive integer up to 365. 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
Expand Down
3 changes: 2 additions & 1 deletion apps/gittensory-ui/public/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -8618,7 +8618,8 @@
"reviewNagCooldownDays": {
"type": "integer",
"minimum": 0,
"exclusiveMinimum": true
"exclusiveMinimum": true,
"maximum": 365
},
"reviewNagLabel": {
"type": "string"
Expand Down
10 changes: 8 additions & 2 deletions src/db/repositories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ import {
upstreamSourceSnapshots,
webhookEvents,
} from "./schema";
import { MAX_REVIEW_NAG_COOLDOWN_DAYS } from "../settings/agent-actions";
import type {
Advisory,
AdvisoryFinding,
Expand Down Expand Up @@ -562,7 +563,7 @@ export async function getRepositorySettings(env: Env, fullName: string): Promise
contributorCapLabel: row.contributorCapLabel,
reviewNagPolicy: normalizeReviewNagPolicy(row.reviewNagPolicy),
reviewNagMaxPings: normalizePositiveIntWithDefault(row.reviewNagMaxPings, 3),
reviewNagCooldownDays: normalizePositiveIntWithDefault(row.reviewNagCooldownDays, 5),
reviewNagCooldownDays: normalizeReviewNagCooldownDays(row.reviewNagCooldownDays, 5),
reviewNagLabel: row.reviewNagLabel,
autoCloseExemptLogins: parseAutoCloseExemptLogins(row.autoCloseExemptLoginsJson),
requireFreshRebaseWindowMinutes: normalizeOpenItemCap(row.requireFreshRebaseWindowMinutes),
Expand Down Expand Up @@ -649,7 +650,7 @@ export async function upsertRepositorySettings(env: Env, settings: Partial<Repos
contributorCapLabel: settings.contributorCapLabel ?? "over-contributor-limit",
reviewNagPolicy: normalizeReviewNagPolicy(settings.reviewNagPolicy),
reviewNagMaxPings: normalizePositiveIntWithDefault(settings.reviewNagMaxPings, 3),
reviewNagCooldownDays: normalizePositiveIntWithDefault(settings.reviewNagCooldownDays, 5),
reviewNagCooldownDays: normalizeReviewNagCooldownDays(settings.reviewNagCooldownDays, 5),
reviewNagLabel: settings.reviewNagLabel ?? "review-nag-cooldown",
autoCloseExemptLogins: normalizeAutoCloseExemptLogins(settings.autoCloseExemptLogins).logins,
requireFreshRebaseWindowMinutes: normalizeOpenItemCap(settings.requireFreshRebaseWindowMinutes),
Expand Down Expand Up @@ -5798,6 +5799,11 @@ function normalizePositiveIntWithDefault(value: number | null | undefined, fallb
return value;
}

function normalizeReviewNagCooldownDays(value: number | null | undefined, fallback: number): number {
const normalized = normalizePositiveIntWithDefault(value, fallback);
return Math.min(normalized, MAX_REVIEW_NAG_COOLDOWN_DAYS);
}

function parseAutonomyPolicy(value: string): AutonomyPolicy {
return normalizeAutonomyPolicy(parseJson<unknown>(value, null));
}
Expand Down
3 changes: 2 additions & 1 deletion src/openapi/schemas.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { z } from "zod";
import { MAX_REVIEW_NAG_COOLDOWN_DAYS } from "../settings/agent-actions";
import { extendZodWithOpenApi } from "@asteasolutions/zod-to-openapi";

extendZodWithOpenApi(z);
Expand Down Expand Up @@ -647,7 +648,7 @@ export const RepositorySettingsSchema = z
contributorCapLabel: z.string().optional(),
reviewNagPolicy: z.enum(["off", "hold", "close"]).optional(),
reviewNagMaxPings: z.number().int().positive().optional(),
reviewNagCooldownDays: z.number().int().positive().optional(),
reviewNagCooldownDays: z.number().int().positive().max(MAX_REVIEW_NAG_COOLDOWN_DAYS).optional(),
reviewNagLabel: z.string().optional(),
autoCloseExemptLogins: z.array(z.string()).optional(),
accountAgeThresholdDays: z.number().int().positive().nullable().optional(),
Expand Down
3 changes: 2 additions & 1 deletion src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ import { aiReviewCacheInputFingerprint } from "../review/ai-review-cache-input";
import {
downgradeCloseToHold,
downgradeMergeToHold,
MAX_REVIEW_NAG_COOLDOWN_DAYS,
isProtectedAutomationAuthor,
planAgentMaintenanceActions,
type PlannedAgentAction,
Expand Down Expand Up @@ -8814,7 +8815,7 @@ async function maybeThrottleReviewNagPing(
/* 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 cooldownDays = Math.min(settings.reviewNagCooldownDays ?? 5, MAX_REVIEW_NAG_COOLDOWN_DAYS);
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
Expand Down
2 changes: 2 additions & 0 deletions src/settings/agent-actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ export const DEFAULT_CONTRIBUTOR_CAP_LABEL = "over-contributor-limit";
// 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";
// Keep the review-nag lookback operationally bounded so repo-controlled config cannot overflow Date arithmetic.
export const MAX_REVIEW_NAG_COOLDOWN_DAYS = 365;
// 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
Expand Down
12 changes: 11 additions & 1 deletion src/signals/focus-manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -741,6 +741,13 @@ function normalizeOptionalString(value: JsonValue | undefined, field: string, wa
return null;
}

// Keep the review-nag lookback operationally bounded so repo-controlled config cannot overflow Date
// arithmetic. Duplicated from settings/agent-actions.ts's own MAX_REVIEW_NAG_COOLDOWN_DAYS (same value,
// same rationale) rather than imported: this module is part of the UI package's typechecked closure, and
// agent-actions.ts transitively imports github/commands.ts -> utils/crypto.ts, pulling a heavier
// GitHub-App-specific dependency chain into the UI build for one small constant.
const MAX_REVIEW_NAG_COOLDOWN_DAYS = 365;

/**
* Parse the optional `settings:` mapping — a partial repository-settings override. Only recognized
* fields are kept; unknown/invalid values are dropped with a warning and never throw.
Expand Down Expand Up @@ -859,7 +866,10 @@ function parseSettingsOverride(value: JsonValue | undefined, warnings: string[])
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;
if (reviewNagCooldownDays !== null && reviewNagCooldownDays <= MAX_REVIEW_NAG_COOLDOWN_DAYS) out.reviewNagCooldownDays = reviewNagCooldownDays;
if (reviewNagCooldownDays !== null && reviewNagCooldownDays > MAX_REVIEW_NAG_COOLDOWN_DAYS) {
warnings.push(`Manifest field "settings.reviewNagCooldownDays" must be at most ${MAX_REVIEW_NAG_COOLDOWN_DAYS}; ignoring it.`);
}
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
Expand Down
4 changes: 4 additions & 0 deletions test/unit/data-spine.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,10 @@ describe("data spine repositories", () => {
// 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 });
await upsertRepositorySettings(env, { repoFullName: "owner/bigwindowrepo", reviewNagMaxPings: 1_000, reviewNagCooldownDays: 1_000_000_000 });
expect(await getRepositorySettings(env, "owner/bigwindowrepo")).toMatchObject({ reviewNagMaxPings: 1_000, reviewNagCooldownDays: 365 });
await env.DB.prepare("update repository_settings set review_nag_cooldown_days = ? where repo_full_name = ?").bind(1_000_000_000, "owner/bigwindowrepo").run();
expect(await getRepositorySettings(env, "owner/bigwindowrepo")).toMatchObject({ reviewNagMaxPings: 1_000, reviewNagCooldownDays: 365 });
expect(updated.slopAiAdvisory).toBe(false);
expect(await getRepoSyncState(env, "missing/repo")).toBeNull();
expect(await getPullRequest(env, "owner/repo", 404)).toBeNull();
Expand Down
3 changes: 3 additions & 0 deletions test/unit/focus-manifest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1382,6 +1382,9 @@ describe("parseFocusManifest settings override + resolveEffectiveSettings", () =
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);
const tooLarge = parseFocusManifest({ settings: { reviewNagCooldownDays: 366 } });
expect(tooLarge.settings.reviewNagCooldownDays).toBeUndefined();
expect(tooLarge.warnings.some((w) => /settings\.reviewNagCooldownDays/.test(w) && /365/.test(w))).toBe(true);
});

it("parses + resolves the account-age throttle settings from the settings: block, overlaying the DB (#2561)", () => {
Expand Down
48 changes: 48 additions & 0 deletions test/unit/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11976,6 +11976,54 @@ describe("queue processors", () => {
expect(seen.closed).toBe(false);
});

it("REGRESSION (gate-flagged): caps an oversized review-nag cooldown at MAX_REVIEW_NAG_COOLDOWN_DAYS before Date arithmetic, even when the resolved settings object itself carries an oversized value", async () => {
// upsertRepositorySettings/getRepositorySettings both clamp reviewNagCooldownDays on write AND read, so
// seeding an oversized value through the normal repository layer (even via a raw DB update bypassing the
// write-time clamp) can never actually reach maybeThrottleReviewNagPing uncapped -- the read-time clamp in
// getRepositorySettings neutralizes it first. Mock resolveRepositorySettings directly so this test proves
// processors.ts's OWN Math.min(reviewNagCooldownDays, MAX_REVIEW_NAG_COOLDOWN_DAYS) guard, not the DB layer.
const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", reviewNagPolicy: "hold", reviewNagMaxPings: 3 });
await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 206, title: "Huge cooldown", state: "open", user: { login: "chatty" }, author_association: "NONE", labels: [], body: "" });
// Three prior pings, all 400 DAYS ago -- outside the 365-day cap, but well within an uncapped
// "1,000,000,000-day" window. If the guard clamps correctly, these fall outside the window and don't
// count; if the guard were removed, the uncapped window would count all three, crossing maxPings=3.
vi.setSystemTime(new Date("2025-04-24T00:00:00.000Z"));
for (let i = 0; i < 3; i += 1) {
await repositoriesModule.recordAuditEvent(env, { eventType: "github_app.review_nag_ping", actor: "chatty", targetKey: "JSONbored/gittensory#206", outcome: "completed" });
}
vi.setSystemTime(new Date("2026-05-29T00:00:00.000Z")); // ~400 days later
const baseSettings = await repositorySettingsModule.resolveRepositorySettings(env, "JSONbored/gittensory");
const resolveSettingsSpy = vi
.spyOn(repositorySettingsModule, "resolveRepositorySettings")
.mockResolvedValueOnce({ ...baseSettings, reviewNagCooldownDays: 1_000_000_000 });
const seen = { comments: [] as string[], labels: [] as string[], closed: false };
stubReviewNagFetch(206, seen);

await processJob(env, {
type: "github-webhook",
deliveryId: "nag-huge-cooldown",
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: "Huge cooldown", state: "open", pull_request: {}, user: { login: "chatty" }, author_association: "NONE" },
comment: { id: 1, body: "@gittensory help", user: { login: "chatty", type: "User" }, author_association: "NONE" },
},
});

// The 400-day-old pings fell outside the CAPPED 365-day window, so this is only the 1st ping this
// window — under maxPings=3, never throttled. An uncapped window would have counted all 3 prior pings
// (pingCount=4 > maxPings=3) and applied the cooldown instead.
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);
expect(seen.closed).toBe(false);
expect(seen.comments.some((c) => c.includes("cooldown limit"))).toBe(false);
expect(resolveSettingsSpy).toHaveBeenCalled();
resolveSettingsSpy.mockRestore();
});

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 });
Expand Down
Loading