Code Review Bench PR #25691 - Switched to time-based domain warmup

[ketkarameya]

Code Review Bench PR TryGhost#25691 - Switched to time-based domain warmup

Benchmark PR for Gitar evaluation

Original PR: agentic-review-benchmarks#9

Changes domain warmup strategy from count-based to time-based approach.

---

Summary by Gitar

• Algorithm Simplification: Replaced complex multi-tier count-based scaling (1.25×, 1.5×, 1.75×, 2× multipliers with special high-volume capping) with a single time-based exponential formula: limit(day) = 200 × 1000^(day/41)
• Deterministic Progression: Email volume limits now progress predictably from 200 messages (day 0) to 200,000 messages (day 41) over 42 days, then unlimited thereafter—independent of historical send counts
• Database Query Optimization: Changed from querying highest historical email count (DESC order) to querying oldest email with csd_email_count (ASC order) to calculate days elapsed since warmup start
• Type Structure Refactoring: Simplified WarmupScalingTable (complex threshold table) to WarmupVolumeOptions (three parameters: start, end, totalDays) reducing code by ~19 lines net
• Test Coverage Enhancement: Updated unit and integration tests with deterministic time-based progression validation at key days (0, 1, 5, 10, 20, 21, 30, 40, 41, 42+) using fake timers for consistency

[gitar-bot-staging]

⚠️ Bug: Off-by-one: day 42 doesn't return Infinity as intended

The condition day > this.#warmupConfig.totalDays (i.e., day > 42) means that on day 42 exactly, the code falls through to the exponential formula instead of returning Infinity. This produces a limit of ~237,137 on day 42, not unlimited sending.

The PR context explicitly states "Day 42+: Infinity (unlimited)" and the unit test at line 284-301 expects Infinity for daysAgo(42). With the current > check, that test would fail because 42 > 42 is false.

The fix is to use >= instead of >:

if (day >= this.#warmupConfig.totalDays) {

This ensures day 42 and beyond correctly return Infinity, matching the documented 42-day warmup period (days 0-41 = 42 days of warming).

_{Was this helpful? React with 👍 / 👎}

Suggested change

	if (day > this.#warmupConfig.totalDays) {
	if (day >= this.#warmupConfig.totalDays) {

• Apply suggested fix

[gitar-bot-staging]

⚠️ This PR is blocked due to unresolved code review findings.

_{Configure merge blocking · Maintainers can dismiss this review. Gitar never approves changes.}

[gitar-bot-staging]

Code Review ⚠️ Changes requested 0 resolved / 2 findings

Clean refactor from count-based to time-based domain warmup, but the off-by-one in the boundary check (> instead of >=) means day 42 doesn't return Infinity as designed—the corresponding unit test would also fail.

⚠️ Bug: Off-by-one: day 42 doesn't return Infinity as intended

📄 ghost/core/core/server/services/email-service/DomainWarmingService.ts:85

The condition day > this.#warmupConfig.totalDays (i.e., day > 42) means that on day 42 exactly, the code falls through to the exponential formula instead of returning Infinity. This produces a limit of ~237,137 on day 42, not unlimited sending.

The PR context explicitly states "Day 42+: Infinity (unlimited)" and the unit test at line 284-301 expects Infinity for daysAgo(42). With the current > check, that test would fail because 42 > 42 is false.

The fix is to use >= instead of >:

if (day >= this.#warmupConfig.totalDays) {

This ensures day 42 and beyond correctly return Infinity, matching the documented 42-day warmup period (days 0-41 = 42 days of warming).

Suggested fix

        if (day >= this.#warmupConfig.totalDays) {

💡 Edge Case: No validation of created_at before date arithmetic

📄 ghost/core/core/server/services/email-service/DomainWarmingService.ts:75

The created_at value is cast directly from unknown to string without validation. If the value is somehow missing, malformed, or not a valid date string, new Date(...) would produce an Invalid Date, and .getTime() would return NaN. This would propagate through the entire calculation, causing getWarmupLimit to return NaN instead of a meaningful limit.

While this is unlikely in practice (records with csd_email_count:-null should always have valid created_at), a simple guard would make the code more defensive:

const createdAt = new Date(res.data[0].get('created_at') as string);
if (isNaN(createdAt.getTime())) {
    return 0;
}
return Math.ceil((Date.now() - createdAt.getTime()) / (1000 * 60 * 60 * 24));

Rules 🎸 1 action taken

Gitar Rules

🎸 Summary Enhancement: Enhanced minimal PR description with 5 technical bullet points covering algorithm simplification, deterministic progression, database optimization, type refactoring, and test coverage

_{5 rules not applicable. Show all rules by commenting gitar display:verbose.}

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}