feat(birmel): make elections weekly on Wednesday

[shepherdjerred]

• Add dayOfWeek config option to ElectionsConfigSchema (default: 3 = Wednesday)
• Update isElectionStartTime to check day of week before starting elections
• Change hasElectionToday to hasElectionThisWeek to prevent duplicate weekly elections
• Update log messages to reflect weekly instead of daily elections

[github-actions]

Claude finished @shepherdjerred's task in 1m 32s —— View job

---

PR Review: Weekly Elections on Wednesday

Summary

This PR successfully converts elections from daily to weekly (Wednesday by default), adds configurable day-of-week support, and switches from runoff elections to random tie-breaking. The implementation is solid with a few minor issues to address.

---

Critical Issues

1. Timezone inconsistency in hasElectionThisWeek() (packages/birmel/src/scheduler/jobs/elections.ts:70-96)

Problem: The function uses now.getDay() which returns the day of week in the system's local timezone, not the configured config.elections.timezone. This creates a race condition where:

• The system could be in a different timezone than configured (e.g., UTC vs America/Los_Angeles)
• Elections could be checked for the wrong week if the system crosses midnight before the configured timezone

Example:

// System time: Sunday 00:30 UTC
// Config timezone: America/Los_Angeles (still Saturday 17:30)
const now = new Date();
const dayOfWeek = now.getDay(); // Returns 0 (Sunday in UTC)
// But in LA timezone it's still Saturday!

Fix: Use the same getCurrentTimeInTimezone() approach:

async function hasElectionThisWeek(guildId: string): Promise<boolean> {
  const config = getConfig();
  const current = getCurrentTimeInTimezone(config.elections.timezone);
  const now = new Date();
  
  // Calculate week boundaries using the configured timezone's day
  const weekStart = new Date(now);
  weekStart.setDate(now.getDate() - current.dayOfWeek);
  // ... rest of calculation
}

---

Design Issues

2. Week calculation uses Sunday-Saturday, not matching election scheduling (packages/birmel/src/scheduler/jobs/elections.ts:74-82)

Observation: The week calculation uses Sunday as the start of the week (lines 74-76), but elections are scheduled for Wednesday. This creates an edge case:

• If an election fails to start on Wednesday due to downtime/errors
• And the bot comes back online Thursday-Saturday
• hasElectionThisWeek() still returns true, preventing retry
• The guild misses their election for the entire week

Consideration: Should the week boundary align with the election day? For example, if elections are on Wednesday, should the "week" run Wednesday-Tuesday? This would allow:

• Failed elections to retry on the next scheduler run (within reason)
• Clearer "election week" semantics

Alternative: Keep current Sunday-Saturday week but add explicit retry logic for failed elections.

---

3. Random winner selection could be more robust (packages/birmel/src/elections/winner.ts:43-45)

Code:

const randomIndex = Math.floor(Math.random() * tiedCandidates.length);
const randomWinner = tiedCandidates[randomIndex] ?? tiedCandidates[0] ?? "jerred";

Issue: The double fallback (?? tiedCandidates[0] ?? "jerred") suggests uncertainty about array access. With noUncheckedIndexedAccess: true in tsconfig, this makes sense. However:

• If tiedCandidates.length > 1 (checked on line 42), then randomIndex is guaranteed to be valid
• The randomIndex can never be out of bounds: Math.floor(Math.random() * 2) gives 0 or 1 for length=2
• The fallbacks are unnecessary defensive code

Suggestion: Either trust the math or add an assertion:

const randomIndex = Math.floor(Math.random() * tiedCandidates.length);
const randomWinner = tiedCandidates[randomIndex];
if (!randomWinner) throw new Error("Unexpected: no tied candidate at valid index");

Or simplify:

// This works because we already checked tiedCandidates.length > 1
const randomWinner = tiedCandidates[Math.floor(Math.random() * tiedCandidates.length)] ?? "jerred";

---

Minor Issues

4. Magic number for election duration (packages/birmel/src/scheduler/jobs/elections.ts:143-149)

Code:

const endTime = new Date(now.getTime() + 2 * 60 * 60 * 1000); // 2 hours from now
// ...
duration: 2, // 2 hours

The duration is hardcoded in two places. Consider:

• Making it a constant: const ELECTION_DURATION_HOURS = 2
• Or better: using the difference between config.elections.startTime and config.elections.endTime to calculate duration dynamically

Current gap: Config has startTime: "17:00" and endTime: "19:00" but the code ignores endTime and hardcodes 2 hours.

---

5. Day-of-week mapping could be centralized (packages/birmel/src/scheduler/jobs/elections.ts:47)

Code:

const dayMap: Record<string, number> = { Sun: 0, Mon: 1, Tue: 2, Wed: 3, Thu: 4, Fri: 5, Sat: 6 };

This is a utility that might be used elsewhere. Consider extracting to a shared utility module if day-of-week parsing is needed in other features (birthdays, scheduled tasks, etc.).

---

Architectural Review

Overall design: ✅ The approach fits well with the existing scheduler pattern. The changes are minimal and focused.

Pattern consistency: ✅ Follows the same check* function pattern used by other scheduled jobs.

Configuration approach: ✅ Using Zod schema with sensible defaults is the right choice.

Database impact: ✅ No schema changes needed - using createdAt for week filtering is appropriate.

---

Commit Messages

First commit: feat(birmel): make elections weekly on Wednesday

• ✅ Clear and descriptive
• ✅ Follows conventional commit format
• ⚠️ Could mention the configurable day aspect: feat(birmel): make elections weekly with configurable day (default Wednesday)

Second commit: fix(birmel): resolve election ties with random winner instead of runoff

• ✅ Good commit separation - this is logically distinct from weekly elections
• ✅ Explains "why" - replacing runoff system
• 🤔 Consider if this should be a separate PR (it's a behavior change independent of weekly elections)

---

Security

✅ No security concerns identified:

• Input validation via Zod schema with proper bounds (0-6 for dayOfWeek)
• No SQL injection risks (using Prisma)
• No XSS risks in Discord messages

---

Testing Recommendations

Consider adding tests for:

1. ✅ isElectionStartTime() with different timezones and days
2. ✅ hasElectionThisWeek() week boundary edge cases (Saturday→Sunday rollover)
3. ✅ Random tie resolution (verify it selects from tied candidates only)
4. ⚠️ Timezone edge case: When system timezone ≠ config timezone

---

Summary

Approve with minor fixes recommended:

• Must fix: Timezone inconsistency in hasElectionThisWeek() (Bump shelljs from 0.7.8 to 0.8.5 in /funsheet/src/main/resources/funsheet-vue #1)
• Should consider: Week boundary alignment (Bump node-sass from 4.7.2 to 7.0.0 in /funsheet/src/main/resources/funsheet-vue #2)
• Nice to have: Simplify random selection (Bump aws-cdk-lib from 2.22.0 to 2.80.0 in /skill-capped-discord-bot #3), use config for duration (Bump got and nodemon in /the-button/api #4)

The core logic is sound and the feature works as intended. The timezone issue is the only critical bug that could cause real problems in production.

[github-actions]

Claude finished @shepherdjerred's task in 1m 31s —— View job

---

PR Review: Weekly Elections Implementation

This PR successfully converts elections from daily to weekly (Wednesday by default), adds configurable day-of-week support, and switches from runoff elections to random tie-breaking. The implementation is well-structured, and the critical timezone bug identified in the previous review has been fixed. Here's my analysis:

---

Architecture & Design ✅

Architectural Fit: The changes integrate cleanly with the existing scheduler pattern. The separation of concerns is maintained:

• Config schema changes are minimal and focused (packages/birmel/src/config/schema.ts:113)
• Election logic stays in dedicated modules
• Follows the same check* function pattern used by other scheduled jobs

Design Quality: The approach is solid:

• Using createdAt timestamp for week filtering is appropriate (no DB schema changes needed)
• The getCurrentTimeInTimezone() helper is well-designed and reusable
• Configuration via Zod schema with sensible defaults (Wednesday = 3) is the right choice

Pattern Consistency: ✅ Matches existing codebase patterns for scheduled tasks and config management.

---

Logic & Edge Cases

✅ Fixed: Timezone Consistency (packages/birmel/src/scheduler/jobs/elections.ts:70-96)

The third commit successfully addresses the timezone bug identified in the previous review. The hasElectionThisWeek() function now correctly uses getCurrentTimeInTimezone() to calculate the day of week in the configured timezone, not the system timezone. This ensures consistency with isElectionStartTime().

Before: Used now.getDay() which returns the system's local timezone day
After: Uses current.dayOfWeek from getCurrentTimeInTimezone() which respects config.elections.timezone

This is the correct fix.

🤔 Design Question: Week Boundary Semantics (packages/birmel/src/scheduler/jobs/elections.ts:74-83)

The week calculation uses Sunday as the start of the week (line 77: weekStart.setDate(now.getDate() - current.dayOfWeek)). This creates an interesting edge case:

Scenario: If an election fails to start on Wednesday due to downtime/errors, and the bot comes back online Thursday-Saturday, hasElectionThisWeek() still returns true, preventing any retry until the next week.

Questions:

1. Is this the desired behavior? (Probably yes - one election per calendar week)
2. Should there be explicit retry logic for failed elections?
3. Consider documenting this behavior in code comments

This isn't necessarily a bug, but it's worth confirming the intended behavior. For most use cases, this is probably fine since the scheduler runs frequently.

⚠️ Minor: Defensive Fallbacks in Random Selection (packages/birmel/src/elections/winner.ts:44-45)

const randomIndex = Math.floor(Math.random() * tiedCandidates.length);
const randomWinner = tiedCandidates[randomIndex] ?? tiedCandidates[0] ?? "jerred";

The double fallback (?? tiedCandidates[0] ?? "jerred") is defensive code that can never be reached:

• We already checked tiedCandidates.length > 1 on line 42
• Math.floor(Math.random() * n) for n >= 2 always returns a valid index (0 to n-1)
• The randomIndex can never be out of bounds

This is harmless defensive coding that plays well with noUncheckedIndexedAccess: true in the tsconfig. Not a bug, just unnecessary - but acceptable for safety.

⚠️ Minor: Hardcoded Election Duration (packages/birmel/src/scheduler/jobs/elections.ts:144-150)

The election duration is hardcoded in two places:

const endTime = new Date(now.getTime() + 2 * 60 * 60 * 1000); // 2 hours from now
// ...
duration: 2, // 2 hours

The config has both startTime: "17:00" and endTime: "19:00", but the code ignores endTime and hardcodes 2 hours.

Suggestion: Consider either:

1. Using config.elections.endTime to calculate duration dynamically
2. Removing endTime from config if it's not used
3. Adding a duration field to the config

This is a minor inconsistency but doesn't affect functionality.

---

Security ✅

No security concerns identified:

• ✅ Input validation via Zod schema with proper bounds (dayOfWeek: 0-6)
• ✅ No SQL injection risks (using Prisma ORM)
• ✅ No XSS risks in Discord messages (using Discord.js API)
• ✅ No command injection vectors
• ✅ Proper error handling prevents information leakage

---

Commit Quality ✅

Commit 1: feat(birmel): make elections weekly on Wednesday

• ✅ Clear, descriptive, follows conventional commit format
• ✅ Body explains all key changes with bullet points
• ✅ Focused on a single feature

Commit 2: fix(birmel): resolve election ties with random winner instead of runoff

• ✅ Good commit separation - logically distinct from weekly elections
• ✅ Explains both "what" and "why" (replacing runoff system)
• ✅ Could arguably be a separate PR (it's an independent behavior change), but acceptable to include here

Commit 3: fix(birmel): use configured timezone for week calculation in hasElectionThisWeek

• ✅ Excellent commit message with clear explanation of the bug
• ✅ Explains the "before/after" state
• ✅ Correctly tagged as a fix

All commits follow conventional commit format and explain the "why" effectively.

---

Code Quality

Strengths:

• ✅ Type-safe with proper Zod validation
• ✅ Good error handling and logging throughout
• ✅ Clear function names and variable names
• ✅ Appropriate use of early returns
• ✅ Proper async/await usage

Minor Observations:

• The dayMap on line 47 could be extracted to a shared utility if day-of-week parsing is needed elsewhere (birthdays, scheduled tasks, etc.). Not critical for now.
• Log messages correctly updated from "daily" to "weekly" terminology

---

Testing Recommendations

Consider adding tests for:

1. ✅ isElectionStartTime() with different timezones and days of week
2. ✅ hasElectionThisWeek() week boundary edge cases (e.g., Saturday→Sunday rollover)
3. ✅ Random tie resolution (verify it selects only from tied candidates)
4. ⚠️ Edge case: System timezone differs from config timezone (now handled correctly in code)
5. ⚠️ Edge case: Failed election on Wednesday, bot restarts Thursday (current behavior: no retry until next week)

---

Summary

Status: ✅ Approve with optional enhancements

What's Working Well:

• Core weekly election logic is sound and well-implemented
• Timezone consistency bug has been fixed (critical issue resolved)
• Random tie-breaking works correctly
• Config schema is well-designed with sensible defaults
• Commit messages are excellent

Optional Improvements (not blocking):

1. Consider documenting the week boundary behavior (Sunday-Saturday, no mid-week retries)
2. Consider calculating duration from config.elections.endTime instead of hardcoding
3. Consider extracting dayMap to a shared utility if reused elsewhere

Recommendation: This PR is ready to merge. The critical timezone bug has been addressed, and the implementation follows good patterns. The optional improvements are minor polish items that can be addressed later if needed.

[github-actions]

Claude finished @shepherdjerred's task in 1m 27s —— View job

---

PR Review: Weekly Elections

This PR successfully converts elections from daily to weekly (Wednesday by default), adds configurable day-of-week support, and switches from runoff elections to random tie-breaking. The implementation is solid with one observation worth discussing.

---

Architectural Fit ✅

Pattern Consistency: The changes integrate cleanly with the existing scheduler pattern. The getCurrentTimeInTimezone() helper is well-designed and reusable, and the week boundary check in hasElectionThisWeek() follows the same date-range query pattern used elsewhere in the codebase.

Configuration Design: Using Zod schema with sensible defaults (Wednesday = 3) is appropriate. The dayOfWeek field correctly uses the JavaScript Date convention (0=Sunday, 6=Saturday) and includes good inline documentation.

Separation of Concerns: Election logic stays in dedicated modules, config changes are minimal and focused. The changes don't leak across architectural boundaries.

---

Logic & Edge Cases

Week Boundary Semantics (packages/birmel/src/scheduler/jobs/elections.ts:70-96)

The week calculation uses Sunday as the start of the week (line 77: weekStart.setDate(now.getDate() - current.dayOfWeek)). This creates an interesting behavior:

Scenario: If an election fails to start on Wednesday due to downtime/errors, and the bot comes back online Thursday-Saturday, hasElectionThisWeek() returns true, preventing any retry until the following week.

Is this a problem? Probably not for most use cases, since:

• The scheduler runs every 5 minutes (based on the 5-minute window on line 67)
• Elections only fail to start if there's a multi-hour outage or config issue
• Having multiple elections per week would violate the "weekly" semantics

Consideration: If you want to allow mid-week retries for failed elections, you could align the week boundary with the election day (e.g., Wednesday-Tuesday instead of Sunday-Saturday). However, this adds complexity and may not be worth it given the frequent scheduler runs.

Current behavior is reasonable - just worth documenting if this edge case comes up.

---

Timezone Consistency ✅ (packages/birmel/src/scheduler/jobs/elections.ts:28-51)

The third commit correctly fixes the timezone bug. Both isElectionStartTime() and hasElectionThisWeek() now use getCurrentTimeInTimezone() to respect the configured timezone. This ensures:

• Day-of-week checks use the configured timezone, not system timezone
• Week boundaries are calculated correctly regardless of where the bot is deployed
• Consistent behavior across all election timing logic

Good fix.

---

Random Winner Selection (packages/birmel/src/elections/winner.ts:44-45)

const randomIndex = Math.floor(Math.random() * tiedCandidates.length);
const randomWinner = tiedCandidates[randomIndex] ?? tiedCandidates[0] ?? "jerred";

The double fallback is defensive code that can't actually be reached (we already checked tiedCandidates.length > 1 on line 42, and Math.floor(Math.random() * n) for n >= 2 always returns a valid index 0 to n-1).

This is fine - it plays well with noUncheckedIndexedAccess: true in the TypeScript config and adds safety without harm. Not a bug.

---

Type Safety in Tie Announcement (packages/birmel/src/scheduler/jobs/elections.ts:258)

The fourth commit adds "send" in channel to the type guard. This is correct - isTextBased() alone doesn't guarantee the send method exists (e.g., thread channels might be text-based but not have direct send). Good defensive programming.

---

Design

Hardcoded Duration vs Config (packages/birmel/src/scheduler/jobs/elections.ts:144-150)

const endTime = new Date(now.getTime() + 2 * 60 * 60 * 1000); // 2 hours from now
// ...
duration: 2, // 2 hours

The config has both startTime: "17:00" and endTime: "19:00", but the code ignores endTime and hardcodes 2 hours. Options:

1. Calculate duration from endTime - startTime
2. Remove endTime from config if unused
3. Add a durationHours field to config

Not critical, but worth considering for consistency.

---

Runoff → Random: Behavioral Change

The second commit removes the runoff system entirely and replaces it with random selection. This is a significant behavioral change (though arguably simpler and better for a Discord bot context).

Justification: Runoff elections would require users to vote twice, which is probably too much friction for a fun Discord bot feature. Random selection is instant and keeps the game moving.

This is the right call, but worth noting it's a breaking change if anyone relied on runoffs.

---

Security ✅

No security concerns:

• Input validation via Zod schema with proper bounds (dayOfWeek: 0-6) ✅
• No SQL injection risks (using Prisma ORM) ✅
• No XSS risks in Discord messages (using Discord.js API) ✅
• Proper error handling prevents information leakage ✅

---

Commit Messages ✅

Commit 1: feat(birmel): make elections weekly on Wednesday

• Clear, descriptive, follows conventional commits format
• Body explains all key changes with bullet points
• Focused on a single feature

Commit 2: fix(birmel): resolve election ties with random winner instead of runoff

• Good commit separation - logically distinct from weekly elections
• Explains "why" (replacing runoff system)
• Could arguably be a separate PR (independent behavior change), but acceptable here

Commit 3: fix(birmel): use configured timezone for week calculation in hasElectionThisWeek

• Excellent commit message explaining the bug and fix
• Clear "before/after" state

Commit 4: fix(birmel): add type guard for channel.send in tie announcement

• Concise and clear
• Addresses a TypeScript type safety issue

All commits follow conventional commit format and explain the "why" effectively.

---

Summary

Status: ✅ Approve

What's Working Well:

• Core weekly election logic is sound and well-implemented
• Timezone handling is correct (critical bug fixed in commit 3)
• Random tie-breaking is simpler and more appropriate than runoffs
• Config schema is well-designed with sensible defaults
• Type safety improvements in commit 4
• Excellent commit messages throughout

Minor Observations (not blocking):

1. Week boundary behavior (Sunday-Saturday, no mid-week retries) is reasonable but could be documented if the edge case comes up
2. Consider calculating duration from config.elections.endTime instead of hardcoding 2 hours

This PR is ready to merge. The implementation follows good patterns, the critical timezone bug has been addressed, and the behavioral changes (weekly + random tie resolution) are appropriate for the use case.

---

[github-actions]

Claude finished @shepherdjerred's task in 1m 21s —— View job

---

PR Review: Weekly Elections

Todo:

• Read repository guidelines (CLAUDE.md)
• Review changed files for context
• Analyze architectural fit
• Check for logic errors and edge cases
• Review security implications
• Evaluate design decisions
• Review commit messages
• Post comprehensive review

---

This PR successfully converts elections from daily to weekly (Wednesday by default), adds configurable day-of-week support, and switches from runoff elections to random tie-breaking. The implementation is clean and well-executed. Here's my focused review on aspects beyond what linters/typecheckers can catch:

Architectural Fit ✅

Pattern Consistency: The changes integrate seamlessly with existing scheduler patterns. The getCurrentTimeInTimezone() helper is well-designed and reusable. Using createdAt timestamps for week filtering is appropriate and avoids unnecessary DB schema changes.

Configuration Design: The Zod schema approach with sensible defaults (Wednesday = 3) aligns perfectly with the codebase's validation strategy. Good inline documentation on the dayOfWeek field.

Logic & Edge Cases

Week Boundary Behavior (elections.ts:70-96)

The week calculation uses Sunday as the start (line 77: weekStart.setDate(now.getDate() - current.dayOfWeek)). This creates a specific behavior worth noting:

Scenario: If an election fails to start on Wednesday due to downtime, and the bot comes back online Thursday-Saturday, hasElectionThisWeek() returns true, preventing any retry until next week.

Assessment: This is probably the desired behavior (one election per calendar week). However, consider:

• The scheduler runs every 5 minutes (based on the 5-minute window on line 67), so outages must be multi-hour to miss an election
• For Discord bot use cases, this is reasonable - you don't want multiple elections per week
• Suggestion: Add a code comment explaining this week boundary behavior to help future maintainers

Timezone Consistency ✅ (elections.ts:28-51)

The critical timezone bug identified in previous reviews has been properly fixed. Both isElectionStartTime() and hasElectionThisWeek() now use getCurrentTimeInTimezone(), ensuring:

• Day-of-week checks respect the configured timezone, not system timezone
• Week boundaries are calculated correctly regardless of deployment location
• Consistent behavior across all election timing logic

This was an important fix.

Hardcoded Duration vs Config (elections.ts:144-150)

const endTime = new Date(now.getTime() + 2 * 60 * 60 * 1000); // 2 hours from now
// ...
duration: 2, // 2 hours

The config has endTime: "19:00" but the code hardcodes 2 hours. Observation: This creates a minor inconsistency - if someone changes endTime in config, it won't affect actual election duration. Consider either:

1. Calculating duration from endTime - startTime
2. Removing endTime from config if it's not used
3. Adding a durationHours field

Not critical, but worth considering for config clarity.

Design Decisions

Runoff → Random: Behavioral Change ✅

The second commit removes runoffs entirely in favor of random selection. Assessment: This is the right call for a Discord bot context. Runoff elections would require:

• Users to vote twice (high friction)
• Additional polling complexity
• More time commitment

Random selection keeps the game fun and moves things along. The tie announcement (elections.ts:254-267) properly informs users about the random selection, maintaining transparency.

Random Selection Implementation (winner.ts:44-45)

const randomIndex = Math.floor(Math.random() * tiedCandidates.length);
const randomWinner = tiedCandidates[randomIndex] ?? tiedCandidates[0] ?? "jerred";

The double fallback is defensive code that technically can't be reached (we already checked tiedCandidates.length > 1). Assessment: This is fine - it plays well with noUncheckedIndexedAccess: true in the TypeScript config and adds safety without harm. The CLAUDE.md guidelines say "Avoid type guards where possible," but this is reasonable defensive programming given the strict TS settings.

Type Safety Enhancement (elections.ts:258)

Commit 4 adds "send" in channel to the type guard. Assessment: Correct. isTextBased() alone doesn't guarantee the send method exists (e.g., some channel types might be text-based but have different APIs). Good defensive programming.

Security ✅

No security concerns identified:

• Input validation via Zod with proper bounds (dayOfWeek: 0-6) ✅
• No SQL injection risks (using Prisma ORM) ✅
• No XSS risks in Discord messages (using Discord.js API) ✅
• Proper error handling prevents information leakage ✅

Commit Messages ✅

Commit 1: feat(birmel): make elections weekly on Wednesday

• Clear, descriptive, follows conventional commits
• Body explains all key changes
• Focused on single feature

Commit 2: fix(birmel): resolve election ties with random winner instead of runoff

• Good separation - logically distinct from weekly elections
• Explains "why" (replacing runoff system)
• Could arguably be separate PR (independent behavioral change), but acceptable here

Commits 3-5: All follow conventional commits format with clear explanations of fixes

All commits explain the "why" effectively.

Summary

Status: ✅ Approve

Strengths:

• Core weekly election logic is sound and well-implemented
• Critical timezone bug properly fixed (commit 3)
• Random tie-breaking is simpler and more appropriate than runoffs
• Config schema well-designed with sensible defaults
• Excellent commit messages throughout
• Type safety improvements

Minor Observations (not blocking):

1. Consider adding code comment explaining week boundary behavior (Sunday-Saturday, no mid-week retries)
2. Consider calculating duration from config.elections.endTime or documenting why it's unused
3. The dayMap (line 47) could be extracted to a shared utility if day-parsing is needed elsewhere (birthdays, scheduled tasks, etc.)

The implementation follows codebase patterns, handles edge cases appropriately, and the behavioral changes (weekly + random tie resolution) are well-suited for the Discord bot use case.

---