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
48 changes: 31 additions & 17 deletions src/db/repositories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -631,8 +631,8 @@ export async function getRepositorySettings(env: Env, fullName: string): Promise
reviewNagLabel: row.reviewNagLabel,
reviewNagMonitoredMentions: parseAutoCloseExemptLogins(row.reviewNagMonitoredMentionsJson),
autoCloseExemptLogins: parseAutoCloseExemptLogins(row.autoCloseExemptLoginsJson),
requireFreshRebaseWindowMinutes: normalizeOpenItemCap(row.requireFreshRebaseWindowMinutes),
accountAgeThresholdDays: normalizeOpenItemCap(row.accountAgeThresholdDays),
requireFreshRebaseWindowMinutes: normalizePositiveIntOrNull(row.requireFreshRebaseWindowMinutes),
accountAgeThresholdDays: normalizePositiveIntOrNull(row.accountAgeThresholdDays),
newAccountLabel: row.newAccountLabel,
commandRateLimitPolicy: normalizeCommandRateLimitPolicy(row.commandRateLimitPolicy),
commandRateLimitMaxPerWindow: normalizePositiveIntWithDefault(row.commandRateLimitMaxPerWindow, 20),
Expand Down Expand Up @@ -751,8 +751,8 @@ export async function upsertRepositorySettings(env: Env, settings: Partial<Repos
reviewNagLabel: settings.reviewNagLabel ?? "review-nag-cooldown",
reviewNagMonitoredMentions: normalizeAutoCloseExemptLogins(settings.reviewNagMonitoredMentions).logins,
autoCloseExemptLogins: normalizeAutoCloseExemptLogins(settings.autoCloseExemptLogins).logins,
requireFreshRebaseWindowMinutes: normalizeOpenItemCap(settings.requireFreshRebaseWindowMinutes),
accountAgeThresholdDays: normalizeOpenItemCap(settings.accountAgeThresholdDays),
requireFreshRebaseWindowMinutes: normalizePositiveIntOrNull(settings.requireFreshRebaseWindowMinutes),
accountAgeThresholdDays: normalizePositiveIntOrNull(settings.accountAgeThresholdDays),
newAccountLabel: settings.newAccountLabel ?? "new-account",
commandRateLimitPolicy: normalizeCommandRateLimitPolicy(settings.commandRateLimitPolicy),
commandRateLimitMaxPerWindow: normalizePositiveIntWithDefault(settings.commandRateLimitMaxPerWindow, 20),
Expand Down Expand Up @@ -2718,14 +2718,16 @@ export async function recordModerationViolation(env: Env, args: { eventType: str
return true;
}

// #gate-flagged: same non-clamping, non-rounding shape as normalizeOpenItemCap, PLUS an upper bound --
// unlike an ordinary open-item cap, this value feeds Date arithmetic on the LIVE close path
// (`Date.now() - violationDecayDays * 86400000`); an unbounded value (e.g. a typo adding extra zeros) can
// overflow into an Invalid Date, and calling .toISOString() on an Invalid Date THROWS, crashing the close.
// Clamped (Math.min), not dropped to null, mirroring normalizeReviewNagCooldownDays' own clamping shape for
// the same "still meaningful, just bounded" family of day-count settings.
// #gate-flagged: same non-rounding shape as normalizePositiveIntOrNull, PLUS its OWN upper bound -- unlike an
// ordinary open-item cap, this value feeds Date arithmetic on the LIVE close path (`Date.now() -
// violationDecayDays * 86400000`); an unbounded value (e.g. a typo adding extra zeros) can overflow into an
// Invalid Date, and calling .toISOString() on an Invalid Date THROWS, crashing the close. Clamped (Math.min),
// not dropped to null, mirroring normalizeReviewNagCooldownDays' own clamping shape for the same "still
// meaningful, just bounded" family of day-count settings. Deliberately calls normalizePositiveIntOrNull, NOT
// normalizeOpenItemCap: the latter's 100-row cap is specific to the live-verification sample budget and has
// nothing to do with this setting's own, much larger MAX_MODERATION_VIOLATION_DECAY_DAYS ceiling.
function normalizeModerationDecayDays(value: number | null | undefined): number | null {
const parsed = normalizeOpenItemCap(value);
const parsed = normalizePositiveIntOrNull(value);
return parsed === null ? null : Math.min(parsed, MAX_MODERATION_VIOLATION_DECAY_DAYS);
}

Expand Down Expand Up @@ -6816,13 +6818,25 @@ function normalizeQualityGateMinScore(value: number | null | undefined): number
return Math.max(0, Math.min(100, Math.round(value)));
}

// A per-contributor open-item cap (#2270) counts discrete open PRs/issues, not a 0-100 score, so unlike
// normalizeQualityGateMinScore it is not rounded — a fractional or non-positive value is a malformed cap
// (there's no such thing as "allow 2.5 open PRs"), so it is dropped to null (no cap). Valid counts are
// clamped to the fixed live-verification sample budget so the cap cannot exceed the rows enforcement sees.
function normalizeOpenItemCap(value: number | null | undefined): number | null {
// A discrete positive count (not a 0-100 score), so unlike normalizeQualityGateMinScore it is not rounded —
// a fractional or non-positive value is malformed (there's no such thing as "allow 2.5 open PRs") and is
// dropped to null. Shared by callers with entirely different upper bounds (or none at all) — see
// normalizeOpenItemCap for the one that clamps to the live-verification sample budget, and
// normalizeModerationDecayDays for one with its own, unrelated ceiling.
function normalizePositiveIntOrNull(value: number | null | undefined): number | null {
if (typeof value !== "number" || !Number.isFinite(value) || !Number.isInteger(value) || value <= 0) return null;
return Math.min(value, MAX_CONTRIBUTOR_OPEN_ITEM_CAP);
return value;
}

// A per-contributor open-item cap (#2270): valid counts are clamped to the fixed live-verification sample
// budget so the cap cannot exceed the rows enforcement sees. Only for caps that are actually enforced against
// that sample (contributorOpenPrCap/contributorOpenIssueCap) — an unrelated positive-int setting that happens
// to reuse the same validation shape must call normalizePositiveIntOrNull directly, not this function, or it
// silently inherits a 100-row ceiling that has nothing to do with its own semantics (gate-flagged: this is
// exactly how normalizeModerationDecayDays's unrelated 3650-day ceiling got clamped down to 100 by mistake).
function normalizeOpenItemCap(value: number | null | undefined): number | null {
const parsed = normalizePositiveIntOrNull(value);
return parsed === null ? null : Math.min(parsed, MAX_CONTRIBUTOR_OPEN_ITEM_CAP);
}

function parsePublicSurface(value: string): RepositorySettings["publicSurface"] {
Expand Down
9 changes: 9 additions & 0 deletions test/unit/data-spine.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,15 @@ describe("data spine repositories", () => {
expect((await getRepositorySettings(env, "owner/rebasewindowrepo")).requireFreshRebaseWindowMinutes).toBe(30); // update persists
await upsertRepositorySettings(env, { repoFullName: "owner/rebasewindowrepo", requireFreshRebaseWindowMinutes: 2.5 as never });
expect((await getRepositorySettings(env, "owner/rebasewindowrepo")).requireFreshRebaseWindowMinutes).toBeNull();
// REGRESSION: this is a minutes-based freshness window, not a live-verification-sample-bounded count like
// contributorOpenPrCap above -- it must NOT inherit that unrelated cap's 100 ceiling.
await upsertRepositorySettings(env, { repoFullName: "owner/rebasewindowrepo", requireFreshRebaseWindowMinutes: 500 });
expect((await getRepositorySettings(env, "owner/rebasewindowrepo")).requireFreshRebaseWindowMinutes).toBe(500);
// #1936 minimum account-age gate: no row and no override both default to null (never enforced); round-trips,
// and (REGRESSION, same reasoning as requireFreshRebaseWindowMinutes above) is not capped at 100.
expect((await getRepositorySettings(env, "missing/repo")).accountAgeThresholdDays).toBeNull();
await upsertRepositorySettings(env, { repoFullName: "owner/accountagerepo", accountAgeThresholdDays: 365 });
expect((await getRepositorySettings(env, "owner/accountagerepo")).accountAgeThresholdDays).toBe(365);
// #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({
Expand Down
Loading