Conversation
fd53be4 to
a94f688
Compare
a94f688 to
0aad1c0
Compare
There was a problem hiding this comment.
Pull request overview
Adds a “Shabbat mode” skill package to the skills engine, enabling NanoClaw to pause message/task/IPC processing during precomputed Shabbat/Yom Tov windows and optionally send candle-lighting reminders.
Changes:
- Adds runtime Shabbat schedule module (
src/shabbat.ts) with fast window lookup + candle lighting notifier. - Adds a schedule generator script (
scripts/generate-zmanim.ts) and unit tests for boundary/candle-lighting behavior. - Provides merge templates (“modify/”) to guard the message loop, task scheduler, and IPC watcher when the skill is applied.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| .claude/skills/add-shabbat-mode/manifest.yaml | Declares skill metadata and structured dependency ops. |
| .claude/skills/add-shabbat-mode/add/src/shabbat.ts | Loads schedule, checks “isShabbat”, candle-lighting notifier logic. |
| .claude/skills/add-shabbat-mode/add/src/shabbat.test.ts | Unit tests for time window boundaries and notifier behavior. |
| .claude/skills/add-shabbat-mode/add/scripts/generate-zmanim.ts | Generates the schedule windows using @hebcal/core. |
| .claude/skills/add-shabbat-mode/modify/src/index.ts | Skill patch template for message-loop guards and post-Shabbat summary behavior. |
| .claude/skills/add-shabbat-mode/SKILL.md | Setup/usage documentation for applying the skill and generating schedules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| structured: | ||
| npm_dependencies: | ||
| "@hebcal/core": "^6.0.8" | ||
| modify_base: |
There was a problem hiding this comment.
manifest.yaml declares @hebcal/core under structured.npm_dependencies, which (per the skills engine) will merge it into the project’s package.json and run npm install. This contradicts the SKILL.md instructions and PR description that @hebcal/core is “installed on demand, not a project dependency”. Consider either removing npm_dependencies (and keeping the manual npm install --no-save flow) or updating the documentation to reflect that the dependency is added to the project (ideally as a devDependency if it’s only used by the generator script).
| )?.[0]; | ||
| if (!mainJid) return; | ||
| const channel = findChannel(channels, mainJid); | ||
| channel?.sendMessage(mainJid, text); |
There was a problem hiding this comment.
notifyMainGroup() calls channel?.sendMessage(...) without await/.catch(). Since sendMessage is async, failures would become unhandled promise rejections (which can terminate the process depending on Node settings). Consider making notifyMainGroup async (returning a Promise) and awaiting the send, or explicitly catching/logging errors when fire-and-forget is intended.
| channel?.sendMessage(mainJid, text); | |
| if (!channel) return; | |
| channel | |
| .sendMessage(mainJid, text) | |
| .catch((err) => { | |
| logger.error( | |
| { err, mainJid }, | |
| 'Failed to send notification message to main group', | |
| ); | |
| }); |
| } catch { | ||
| logger.info('No Shabbat schedule found, Shabbat mode disabled'); |
There was a problem hiding this comment.
initShabbatSchedule() swallows all read/parse errors and logs "No Shabbat schedule found" even for JSON corruption or permission issues. This can silently disable Shabbat mode in production and make it hard to diagnose. Consider only treating ENOENT as "not found" and logging a warn/error (with the underlying error) for other failures, especially JSON parse errors.
| } catch { | |
| logger.info('No Shabbat schedule found, Shabbat mode disabled'); | |
| } catch (err: unknown) { | |
| const e = err as NodeJS.ErrnoException; | |
| if (e.code === 'ENOENT') { | |
| logger.info('No Shabbat schedule found, Shabbat mode disabled'); | |
| } else { | |
| logger.error( | |
| { err: e, filePath }, | |
| 'Failed to load Shabbat schedule; Shabbat mode disabled', | |
| ); | |
| } |
| schedule = s; | ||
| windowStarts = s.windows.map((w) => new Date(w.start).getTime()); | ||
| windowEnds = s.windows.map((w) => new Date(w.end).getTime()); |
There was a problem hiding this comment.
isShabbatOrYomTov()/getNextCandleLighting() rely on windows being sorted by start, but loadSchedule() just maps the JSON order without sorting or validating. If the schedule file is generated/edited out of order, the binary search can return incorrect results. Consider sorting windows by start time (and rebuilding windowStarts/windowEnds from the sorted list) when loading the schedule.
| schedule = s; | |
| windowStarts = s.windows.map((w) => new Date(w.start).getTime()); | |
| windowEnds = s.windows.map((w) => new Date(w.end).getTime()); | |
| // Ensure windows are sorted by start time so binary search works correctly | |
| const windows = [...s.windows].sort( | |
| (a, b) => new Date(a.start).getTime() - new Date(b.start).getTime(), | |
| ); | |
| // Store the schedule with windows in sorted order | |
| schedule = { ...s, windows }; | |
| // Build lookup arrays from the sorted windows | |
| windowStarts = windows.map((w) => new Date(w.start).getTime()); | |
| windowEnds = windows.map((w) => new Date(w.end).getTime()); |
| 1. **Location** — latitude, longitude, and timezone for zmanim calculation. No default — the user must provide their location. | ||
| 2. **Israel or Diaspora** — determines Yom Tov observance. Israel keeps 1-day Yom Tov, diaspora keeps 2 days. | ||
| 3. **Candle lighting notifications** — send a reminder to the user every erev Shabbat and erev Yom Tov with the candle lighting time? Default: yes. | ||
| 4. **Elevation** — meters above sea level. Default: 0m. |
There was a problem hiding this comment.
SKILL.md states the default elevation is 0m, but generate-zmanim.ts defaults SHABBAT_ELEVATION to 25. This mismatch can lead to surprising output and makes the setup guide less trustworthy. Consider aligning the script default with the documented default (or updating the doc to match the script).
| 4. **Elevation** — meters above sea level. Default: 0m. | |
| 4. **Elevation** — meters above sea level. Default: 25m. |
| export function startCandleLightingNotifier( | ||
| notify: (text: string) => void, | ||
| ): void { | ||
| if (!schedule) return; | ||
|
|
||
| const check = () => { | ||
| const next = getNextCandleLighting(); | ||
| if (!next) return; | ||
|
|
||
| const windowStart = | ||
| next.time.getTime() + CANDLE_LIGHTING_MINUTES * 60 * 1000; | ||
| if (windowStart === lastNotifiedStart) return; | ||
|
|
||
| const timeUntil = next.time.getTime() - Date.now(); | ||
| if (timeUntil > 0 && timeUntil <= NOTIFY_HORIZON_MS) { | ||
| lastNotifiedStart = windowStart; | ||
| const timeStr = formatTime(next.time); | ||
| const label = next.label.includes('Shabbat') | ||
| ? 'Shabbat Shalom! ' | ||
| : `${next.label} — `; | ||
| notify(`${label}Candle lighting at ${timeStr}`); | ||
| } |
There was a problem hiding this comment.
startCandleLightingNotifier() calls notify(...) without awaiting or handling rejections. In the intended usage, callers are likely to pass an async sendMessage function (returns a Promise), which would create unhandled promise rejections on failure. Consider typing notify as returning Promise<void> (or void | Promise<void>) and explicitly handling failures (e.g., awaiting in an async check or catching/logging).
| if (expiresAt < warningThreshold) { | ||
| logger.warn( | ||
| { expiresAt: parsed.expiresAt }, | ||
| 'Shabbat schedule expires soon! Run: npm run generate-zmanim', |
There was a problem hiding this comment.
The warning string suggests npm run generate-zmanim, but this PR adds scripts/generate-zmanim.ts without adding a corresponding generate-zmanim npm script to package.json. This is likely to confuse operators when the schedule nears expiry. Consider either updating the message to the actual command (e.g., via npx tsx scripts/generate-zmanim.ts), or updating the skill to also add the npm script.
| 'Shabbat schedule expires soon! Run: npm run generate-zmanim', | |
| 'Shabbat schedule expires soon! Run: npx tsx scripts/generate-zmanim.ts', |
| describe('isShabbatOrYomTov', () => { | ||
| beforeEach(() => { | ||
| _loadScheduleForTest(TEST_SCHEDULE); | ||
| }); | ||
| afterEach(() => { | ||
| vi.useRealTimers(); | ||
| }); | ||
|
|
||
| it('returns true during a Shabbat window', () => { | ||
| vi.setSystemTime(new Date('2026-02-20T20:00:00.000Z')); | ||
| expect(isShabbatOrYomTov()).toBe(true); | ||
| }); | ||
|
|
||
| it('returns true at exact start of window (shkiya)', () => { | ||
| vi.setSystemTime(new Date('2026-02-20T17:20:00.000Z')); | ||
| expect(isShabbatOrYomTov()).toBe(true); | ||
| }); | ||
|
|
||
| it('returns false just before shkiya', () => { | ||
| vi.setSystemTime(new Date('2026-02-20T17:19:59.999Z')); | ||
| expect(isShabbatOrYomTov()).toBe(false); | ||
| }); | ||
|
|
||
| it('returns true just before end of window', () => { | ||
| vi.setSystemTime(new Date('2026-02-21T23:44:59.999Z')); | ||
| expect(isShabbatOrYomTov()).toBe(true); | ||
| }); | ||
|
|
||
| it('returns false at exact end of window (tzeis + 18)', () => { | ||
| vi.setSystemTime(new Date('2026-02-21T23:45:00.000Z')); | ||
| expect(isShabbatOrYomTov()).toBe(false); | ||
| }); | ||
|
|
||
| it('returns false on a weekday', () => { | ||
| vi.setSystemTime(new Date('2026-02-24T12:00:00.000Z')); | ||
| expect(isShabbatOrYomTov()).toBe(false); | ||
| }); | ||
|
|
||
| it('returns true during a merged shabbat+yomtov window', () => { | ||
| vi.setSystemTime(new Date('2026-03-21T12:00:00.000Z')); | ||
| expect(isShabbatOrYomTov()).toBe(true); | ||
| }); | ||
|
|
||
| it('returns false before any windows', () => { | ||
| vi.setSystemTime(new Date('2025-01-01T00:00:00.000Z')); | ||
| expect(isShabbatOrYomTov()).toBe(false); | ||
| }); | ||
|
|
||
| it('returns false after all windows', () => { | ||
| vi.setSystemTime(new Date('2027-01-01T00:00:00.000Z')); | ||
| expect(isShabbatOrYomTov()).toBe(false); | ||
| }); |
There was a problem hiding this comment.
shabbat.test.ts uses vi.setSystemTime(...) in the first two suites without enabling fake timers. In Vitest, setSystemTime requires vi.useFakeTimers() (otherwise it typically throws or has no effect), which would make these tests flaky or fail. Consider calling vi.useFakeTimers() in beforeEach for these suites (and vi.useRealTimers() in afterEach), similar to the notifier tests.
| for (let year = startYear; year <= endYear; year++) { | ||
| const events = HebrewCalendar.calendar({ | ||
| year, | ||
| isHebrewYear: false, | ||
| il: false, | ||
| mask: flags.CHAG, | ||
| }); |
There was a problem hiding this comment.
The generator hard-codes il: false, so it always generates diaspora Yom Tov dates. This conflicts with the skill docs/PR description that mention an Israel vs diaspora preference (SHABBAT_IL) and will produce wrong schedules for Israel users. Consider reading an env var/CLI flag (e.g. SHABBAT_IL) and wiring it into the HebrewCalendar.calendar({ il: ... }) call.
0aad1c0 to
e890a4f
Compare
Summary
Ever dream of a kosher lobster? This PR lets your crustacean-named bot keep Shabbat. Every Friday you disconnect to reconnect; now your lobster can too.
From sunset on Friday through motzei Shabbat — and the same for every Yom Tov — nanobot goes completely dark. No containers spawn, no scheduled tasks fire, no IPC gets processed. Messages that arrive during the quiet hours are tucked away in the database, waiting patiently.
When Shabbat ends, NanoClaw wakes up, sends a "Shavua Tov! 🌙" message with a per-group recap of what you missed, and gets ready to take on the week.
It also sends a candle lighting reminder on Fridays so you're always right on time.
How it works
A one-time script generates 5 years of shabbat times for your location using
@hebcal/core(installed on demand, not a project dependency). The output is a flat JSON file of time windows underdata/.At runtime,
isShabbatOrYomTov()does a binary search against the pre-computed windows. It's called on every poll cycle so it needs to be fast — and binary search over a sorted list is about as fast as you get. Three guard points (message loop, task scheduler, IPC watcher) all check this and go to sleep when it returnstrue.If no schedule file exists, the feature is completely inert — no restrictions, no errors, no guilt.
Setup
In Claude Code, run
/setup— it'll ask for your location and preferences, generate the schedule, and you're good to go.Test plan
"And the seventh day is a Sabbath to the Eternal your Almighty. On that day you shall not do any work - you, your son, your daughter, your agents, your livestock, and the guest who dwells among you." - Exodus 20:10
🤖 Generated with Claude Code