Open
Conversation
isair
added a commit
that referenced
this pull request
May 3, 2026
Adversarial review of #304 surfaced two medium issues and several low- severity follow-ups. All addressed: - listener.py: drop the hardcoded English "10-minute pasta timer" example from the startup banner (CLAUDE.md forbids hardcoded language patterns). - llm_contexts.md: document _DIGEST_SKIP_TOOLS in section 5 and call out that timer rides it alongside getWeather, so the digest skip is no longer invisible to the LLM-context map. - ipc_constants.py: new shared module owning TIMER_ALARM_IPC_PREFIX so the daemon side (timer.py) and the desktop side (timer_alarm_dialog.py) cannot drift; previous duplicate definitions removed. - app.py: tighten the IPC fast-path from `prefix in line` to `line.lstrip().startswith(prefix)` so a quoted log line containing the marker can never reach the parser. - timer.py (TimerManager.start): floor sub-second durations to 1 so the alarm dialogue and TTS no longer announce "0 seconds elapsed" on fast test timers (the real countdown still uses the original float). - timer.py (AlarmRegistry.start): register the alarm in `_alarms` BEFORE starting the auto-stop threading.Timer, closing the narrow race where the cap could fire and find nothing to stop. - tests/test_timer_alarm_dialog.py: 23 new tests covering parse_alarm_event, the full TimerAlarmDialog handler surface (no QApplication construction, to avoid Windows access violations from upstream TTS-test pollution), the _handle_timer_alarm_line dispatch logic, and an AST regression guard that prevents the prefix check from regressing back to substring match. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a built-in `timer` tool so Jarvis can set, list, and cancel
countdown timers ("set a timer for 10 minutes", "cancel the pasta
timer", "what timers are running?"). Multiple labelled timers can
run concurrently. When a timer elapses, an in-process TimerManager
announces it audibly via the global TTS engine and visibly by
flipping the desktop face into SPEAKING and printing a stdout
banner; each side-effect is best-effort so headless / eval / test
runs degrade gracefully.
Closes #303
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Restore desktop face to IDLE via TTS completion_callback (with inline safety net) so it never sticks on SPEAKING. - Accept localised `announcement` from the reply LLM; fall back to English only when none provided. Keeps language handling on the LLM per the project's no-hardcoded-patterns rule. - Skip tool digesting for `timer` so ids/etas survive verbatim for follow-up cancel-by-id calls. - Sanitise `label` and `announcement` (collapse whitespace) to prevent render-payload spoofing. - Sum durations as floats and round once so 0.5 minutes -> 30s instead of truncating to 0. - Add timer.spec.md and register it in CLAUDE.md. - Pluggable TTS provider in timer module avoids importing the heavy daemon stack from tests.
- Spawn the threading.Timer OS thread outside the manager lock so start() honours the spec's "expensive work runs unlocked" rule. - Round duration once at the manager boundary so a fractional value no longer sets duration_sec=0 while the underlying timer still fires; sub-second test durations remain valid via the raw float. - Strip `,` and `=` from labels so they cannot smuggle an extra `label=` token into the comma/equals-delimited render payload that the reply LLM parses. - Add tool-error tests for the 24h and max-active hard limits. - Add a hands-free timer example to the README's example dialogues.
Gemma-class small models routinely append 'the N minutes are up' or 'is complete' right after setting a timer, because their language prior links 'timer for N minutes' with 'N minutes have elapsed'. The existing 'Timer status: set' line wasn't load-bearing enough. Add an explicit, unmistakable instruction to the SET payload that the timer is still counting down and the assistant must not claim elapsed state. Covered by a new test that asserts the anchoring phrases are present in the rendered payload.
When a timer fires, the daemon now also drives a looping alarm tone via a desktop dialogue (TimerAlarmDialog with QSoundEffect) alongside the existing stdout banner, face SPEAKING flip, and TTS announcement. Dismiss paths (any one stops the noise and closes the dialogue): - Dismiss button or window close on the dialogue - "stop" voice command (stop tool now silences alarms before exit) - Setting another timer or cancelling timers via the timer tool - 30-second hard cap, enforced by both daemon and dialogue independently Daemon-side AlarmRegistry tracks active alarms and emits __TIMER_ALARM__ IPC lines over stdout (mirroring the diary IPC pattern) so the desktop app can intercept them on the existing log channel. A short BEL fallback covers headless callers when no UI is listening. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Use the daemon-supplied auto_stop_sec from the IPC payload instead of hardcoding 30s in the dialogue, so the daemon stays the single source of truth for the cap. - Drop the unused TimerAlarmSignals class. - Multi-alarm dialogue now shows "Multiple timers done" with a comma-separated label list rather than reusing the latest label, which was misleading. - Inject the BEL fallback writer instead of monkeypatching real sys.stdout in tests, and gate the loop on sys.stdout.isatty() so it doesn't run uselessly when the desktop app captures stdout. - Add tests for the no-TTY skip path and the broken-writer path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…larms Stop events for individual alarms now re-render the title and body so the user no longer sees "3 alarms ringing: 'A', 'B', 'C'" after one of them was dismissed. Per-alarm metadata (label + duration) is now tracked in _active so the rerender path has everything it needs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adversarial review of #304 surfaced two medium issues and several low- severity follow-ups. All addressed: - listener.py: drop the hardcoded English "10-minute pasta timer" example from the startup banner (CLAUDE.md forbids hardcoded language patterns). - llm_contexts.md: document _DIGEST_SKIP_TOOLS in section 5 and call out that timer rides it alongside getWeather, so the digest skip is no longer invisible to the LLM-context map. - ipc_constants.py: new shared module owning TIMER_ALARM_IPC_PREFIX so the daemon side (timer.py) and the desktop side (timer_alarm_dialog.py) cannot drift; previous duplicate definitions removed. - app.py: tighten the IPC fast-path from `prefix in line` to `line.lstrip().startswith(prefix)` so a quoted log line containing the marker can never reach the parser. - timer.py (TimerManager.start): floor sub-second durations to 1 so the alarm dialogue and TTS no longer announce "0 seconds elapsed" on fast test timers (the real countdown still uses the original float). - timer.py (AlarmRegistry.start): register the alarm in `_alarms` BEFORE starting the auto-stop threading.Timer, closing the narrow race where the cap could fire and find nothing to stop. - tests/test_timer_alarm_dialog.py: 23 new tests covering parse_alarm_event, the full TimerAlarmDialog handler surface (no QApplication construction, to avoid Windows access violations from upstream TTS-test pollution), the _handle_timer_alarm_line dispatch logic, and an AST regression guard that prevents the prefix check from regressing back to substring match. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a built-in
timertool for countdowns:set/list/cancelactions, optional labels, concurrent timers, and a best-effort audible + visible announcement when each timer elapses. In-process only (no persistence) per the privacy-first stance. Closes #303.Behaviour
set: integerhours/minutes/seconds(any combination, summed), optionallabel, optionalannouncement(LLM-localised spoken text). Returns id, label, duration, ETA.list: active timers sorted by ETA.cancel: bytimer_id, bylabel(case-insensitive, matches all), orall=true. Cancelling a non-existent timer is a successful no-op.announcementstring. The default announcer fires three best-effort side-effects (stdout banner, desktop face flipped to SPEAKING then restored to IDLE via TTScompletion_callback, and TTS speech). English fallback only kicks in when noannouncementis passed.Review fixes (v2 + v3)
completion_callbackand inline if TTS is unavailable.announcement.timeradded to_DIGEST_SKIP_TOOLSso ids/etas survive verbatim for follow-up cancel calls.labelandannouncementare whitespace-collapsed; labels also strip,and=to prevent render-payload spoofing.0.5 minutesbecomes30s.threading.Timer.start()now runs outside the manager lock, matching the spec's "expensive work runs unlocked" rule.timer.spec.mddocumenting the contract; registered in CLAUDE.md spec table.Review fixes (v4: full adversarial-review pass)
"Set a 10-minute pasta timer"example from the startup banner._DIGEST_SKIP_TOOLSin section 5 and the timer's place in it.TIMER_ALARM_IPC_PREFIX; the daemon side and the desktop side now import the same constant rather than redefining it twice._handle_timer_alarm_line: tightened the IPC fast-path fromprefix in linetoline.lstrip().startswith(prefix)so a quoted log line containing the marker can never reach the parser.TimerManager.start: floor sub-second durations to 1 so the alarm dialogue / TTS no longer announce"0 seconds elapsed"on fast test timers (the real countdown still uses the float).AlarmRegistry.start: register the alarm in_alarmsBEFORE starting the auto-stopthreading.Timer, closing the narrow race where the cap could fire and find nothing to stop.parse_alarm_event, the fullTimerAlarmDialoghandler surface, the_handle_timer_alarm_linedispatch logic, and an AST regression guard that prevents the prefix check from regressing back to substring match. Tests construct the dialogue without a realQApplicationto avoid Windows access violations from unrelated TTS-test pollution.Test plan
parse_alarm_event,TimerAlarmDialog(handle_start / handle_stop / dismiss / render-labels / cap fallbacks), and the_handle_timer_alarm_linedispatch (start / stop / malformed / unknown / substring-spoof rejection).startswith.