Open
Conversation
- New schedule_type='daily_at' with schedule_value='HH:MM' or 'HH:MM,HH:MM,...'
- Daemon fires task if any target time has passed today and last_run predates it
- Restart-safe: daemon downtime at fire-time still fires on next wakeup
- Wizard: new 'Daily at specific time(s)...' menu option with HH:MM validation
- Wizard: friendly summary display ('daily at 09:00,17:00')
- cron remains as a stub with warning (would need croniter)
Covers parse_daily_at_times() and the daily_at branch of should_run_task(): - Single/multiple/whitespace-padded time parsing - Edge cases: midnight, 23:59, empty string, invalid formats - Invalid entries skipped with warning, valid entries still used - Never-run task fires after target, not before (boundary inclusive) - Does not re-fire after running today - Fires when last_run was yesterday (restart-safe) - Fires when daemon missed the window and caught up later - last_run before target but same day still triggers - Multi-time: first due + second future, first ran + second future/due - Disabled tasks never fire - All-invalid schedule_value returns False with warning - Midnight target edge case
- Fix test_wizard_code_puppy_first: 'Daily' was removed from schedule_map when daily_at replaced it; switched to 'Every hour' - Add test_daily_at_single_time: single HH:MM → schedule_type=daily_at - Add test_daily_at_multiple_times: comma list preserved verbatim - Add test_daily_at_cancel_time_input: None from TextInputMenu → None - Add test_daily_at_all_invalid_times: no valid times → None - Add test_daily_at_strips_invalid_times: bad entries dropped, valid kept - Add test_daily_at_summary_display: confirms 'daily at HH:MM' in stdout and raw 'daily_at' type string does not leak into summary
…t prompt Add '(24-hour clock: 1:00 PM = 13:00, 5:30 PM = 17:30, midnight = 00:00)' hint so users who don't think natively in 24-hour time don't have to guess.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a "daily_at" schedule type: the wizard collects one or more HH:MM times (validated, deduplicated, canonicalized, stored comma-separated) and the daemon parses those times and compares them to now and last_run to decide restart-safe task execution. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant Wizard as "Scheduler Wizard\n(code_puppy/plugins/.../scheduler_wizard.py)"
participant Config as "ScheduledTask\n(code_puppy/scheduler/config.py)"
participant Daemon as "Scheduler Daemon\n(code_puppy/scheduler/daemon.py)"
User->>Wizard: choose "Daily at specific time(s)..."
Wizard->>User: prompt for HH:MM times (one or more)
User-->>Wizard: submits "08:00, 18:30" or cancels
Wizard->>Wizard: validate, normalize, dedupe times
Wizard->>Config: create/update ScheduledTask(schedule_type="daily_at", schedule_value="08:00,18:30")
Daemon->>Config: load ScheduledTask
Daemon->>Daemon: parse_daily_at_times("08:00,18:30") -> [(8,0),(18,30)]
Daemon->>Daemon: compute target datetimes, compare to now & last_run
Daemon-->>Config: trigger task execution if a target is due
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code_puppy/scheduler/daemon.py`:
- Around line 28-46: The docstring for parse_daily_at_times is inaccurate: it
says invalid entries are "silently skipped" but the function prints a warning
for each invalid entry; update the docstring to state that invalid entries are
skipped and a warning is printed (e.g., "Invalid entries are skipped and a
warning is printed using print()") and mention the format/behavior consistently
with the existing implementation that prints "[Scheduler] Warning: Invalid time
'{entry}' in daily_at schedule, skipping." to aid future maintainers.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code_puppy/plugins/scheduler/scheduler_wizard.pycode_puppy/scheduler/config.pycode_puppy/scheduler/daemon.pytests/plugins/test_scheduler_wizard.pytests/scheduler/test_daily_at.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/plugins/test_scheduler_wizard.py (1)
595-631: Add one case for blank time input using the default value.A dedicated test for empty user input (
"") would lock in the default-time behavior (09:00) explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/plugins/test_scheduler_wizard.py` around lines 595 - 631, Add a new unit test that mirrors test_daily_at_single_time but sets the TextInputMenu for the time (the second text_instances entry) to return an empty string ("") to assert the wizard uses the default "09:00"; specifically, call create_task_wizard with mocked TextInputMenu instances (first for name, second for blank time, third for working_dir), SelectionMenu returning "Daily at specific time(s)..." and model/agent mocks as in existing tests, then assert result["schedule_type"] == "daily_at" and result["schedule_value"] == "09:00".code_puppy/plugins/scheduler/scheduler_wizard.py (1)
247-270: Consider normalizing and de-duplicating accepted times before storage.This keeps
schedule_valuecanonical (HH:MM) and avoids repeated times in persisted config.♻️ Suggested refinement
- valid = [] + valid = [] + seen = set() for entry in entries: try: - datetime.strptime(entry, "%H:%M") # noqa: DTZ007 - valid.append(entry) + normalized = datetime.strptime(entry, "%H:%M").strftime("%H:%M") # noqa: DTZ007 + if normalized not in seen: + seen.add(normalized) + valid.append(normalized) except ValueError: print(f" ⚠️ Skipping invalid time: '{entry}' (expected HH:MM)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code_puppy/plugins/scheduler/scheduler_wizard.py` around lines 247 - 270, In the "Daily at specific time(s)..." branch, after validating entries (variables: entries, valid), normalize each accepted time by parsing with datetime.strptime and reformatting with strftime("%H:%M"), deduplicate them (e.g., use a set), sort chronologically, and then set schedule_value to the canonical comma-joined string of these normalized times (replace the current ",".join(valid) assignment); keep schedule_type = "daily_at" unchanged and ensure empty-result handling remains the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code_puppy/plugins/scheduler/scheduler_wizard.py`:
- Around line 247-270: In the "Daily at specific time(s)..." branch, after
validating entries (variables: entries, valid), normalize each accepted time by
parsing with datetime.strptime and reformatting with strftime("%H:%M"),
deduplicate them (e.g., use a set), sort chronologically, and then set
schedule_value to the canonical comma-joined string of these normalized times
(replace the current ",".join(valid) assignment); keep schedule_type =
"daily_at" unchanged and ensure empty-result handling remains the same.
In `@tests/plugins/test_scheduler_wizard.py`:
- Around line 595-631: Add a new unit test that mirrors
test_daily_at_single_time but sets the TextInputMenu for the time (the second
text_instances entry) to return an empty string ("") to assert the wizard uses
the default "09:00"; specifically, call create_task_wizard with mocked
TextInputMenu instances (first for name, second for blank time, third for
working_dir), SelectionMenu returning "Daily at specific time(s)..." and
model/agent mocks as in existing tests, then assert result["schedule_type"] ==
"daily_at" and result["schedule_value"] == "09:00".
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code_puppy/plugins/scheduler/scheduler_wizard.pycode_puppy/scheduler/daemon.pytests/plugins/test_scheduler_wizard.py
🚧 Files skipped from review as they are similar to previous changes (1)
- code_puppy/scheduler/daemon.py
nhicks00
requested changes
Feb 28, 2026
Collaborator
There was a problem hiding this comment.
-
The old "Daily" option (run every 24h) got removed from the wizard — was that intentional? Existing daily tasks still work in the daemon but users can't create new ones through the wizard anymore. Should both
options coexist, or is this a deliberate replacement? -
daily_at introduces wall-clock time semantics into a scheduler that's entirely timezone-naive (datetime.now() without tz=). For intervals that's fine, but "run at 09:00" is inherently timezone-dependent. Have
you thought about at least dropping a code comment on the daily_at branch in should_run_task() acknowledging the naive-datetime limitation? Not asking you to solve TZ support here, just document it. -
parse_daily_at_times docstring says "Invalid entries are silently skipped" but the implementation prints a [Scheduler] Warning: — update the docstring to match. (CodeRabbit flagged this too, +1 on their inline
comment.) -
Normalize times with strftime("%H:%M") after parsing and deduplicate — 9:00,09:00 would store duplicates in the config as-is. Won't cause double-firing but it's messy in persisted state. (Same as CodeRabbit's
suggestion on the wizard.) -
Both CI failures (ruff format + pytest) are pre-existing on main — unrelated files, not introduced by this PR.
- Restore 'Daily (every 24h)' wizard option — it was accidentally dropped when daily_at was introduced; interval-based daily tasks still work in the daemon but users could no longer create new ones via the wizard - Fix parse_daily_at_times docstring: 'silently skipped' was wrong — the implementation has always printed a [Scheduler] Warning to stdout - Add TIMEZONE NOTE comment to the daily_at branch in should_run_task() acknowledging that datetime.now() is naive (system local time) and that DST / tzdata changes will shift fire times accordingly - Normalise times to HH:MM canonical form via strftime after parsing and deduplicate — '9:00,09:00,17:00,17:00' now stores as '09:00,17:00' - Tests: add test_daily_at_normalises_single_digit_hour, test_daily_at_deduplicates_times, and Daily (every 24h) to test_schedule_map parametrize (75 tests, all green)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code_puppy/plugins/scheduler/scheduler_wizard.py (1)
259-271: Consider centralizing HH:MM parsing/validation to avoid rule drift.Wizard and daemon each implement time parsing separately. Extracting a shared helper (e.g., under
code_puppy/scheduler) would reduce maintenance risk if validation rules evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code_puppy/plugins/scheduler/scheduler_wizard.py` around lines 259 - 271, The time-parsing/deduplication logic in scheduler_wizard.py (variables raw_times, entries, seen, valid and the datetime.strptime("%H:%M") usage producing normalised values) is duplicated elsewhere; extract it into a shared helper (e.g., a function named parse_times_hhmm that accepts a raw comma-separated string and returns a deduplicated list of canonical "HH:MM" strings), move it into the common scheduler module, and replace the inline loop in scheduler_wizard.py (and the daemon implementation) to call that helper; ensure the helper preserves the current behavior (strip entries, ignore empty/invalid values, normalize with t.strftime("%H:%M"), dedupe while preserving order, and surface/return errors or skip invalid entries consistently).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code_puppy/scheduler/daemon.py`:
- Around line 120-123: The daily_at logic only checks targets for today, so if
last_run is before yesterday and the daemon restarts before today's first target
it won’t catch missed runs; update the daily_at check (the loop that iterates
over times and computes target using now.replace with hour/minute) to also
consider the same targets on the previous day (or, more robustly, consider
targets in the past 24 hours) by creating candidate targets for now and now - 1
day and returning True if any candidate target <= now and (last_run is None or
last_run < candidate_target); keep uses of now, last_run, times, and target to
locate and modify the code.
---
Nitpick comments:
In `@code_puppy/plugins/scheduler/scheduler_wizard.py`:
- Around line 259-271: The time-parsing/deduplication logic in
scheduler_wizard.py (variables raw_times, entries, seen, valid and the
datetime.strptime("%H:%M") usage producing normalised values) is duplicated
elsewhere; extract it into a shared helper (e.g., a function named
parse_times_hhmm that accepts a raw comma-separated string and returns a
deduplicated list of canonical "HH:MM" strings), move it into the common
scheduler module, and replace the inline loop in scheduler_wizard.py (and the
daemon implementation) to call that helper; ensure the helper preserves the
current behavior (strip entries, ignore empty/invalid values, normalize with
t.strftime("%H:%M"), dedupe while preserving order, and surface/return errors or
skip invalid entries consistently).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code_puppy/plugins/scheduler/scheduler_wizard.pycode_puppy/scheduler/daemon.pytests/plugins/test_scheduler_wizard.py
added 2 commits
February 28, 2026 13:18
…ours The original logic only evaluated targets against today's date, so if the daemon was down overnight and restarted before today's first target time, any missed run from yesterday was silently dropped. Fix: for each scheduled HH:MM, check both today's and yesterday's candidate targets. Return True if any candidate is in the past and last_run hasn't reached it yet. Guard: the 24-hour lookback is skipped when last_run is None (brand-new task). A new task created at 07:30 with a 09:00 target should wait for 09:00, not immediately claim yesterday's missed window. Scenario that was broken: target=09:00, last_run=Feb 24 09:02, now=Feb 26 07:30 Old: today's 09:00 not yet reached → False (missed Feb 25 silently) New: Feb 25 09:00 is past and last_run < Feb 25 09:00 → True ✓ Tests added (78 total, all green): test_daemon_restart_catches_missed_yesterday_target test_new_task_does_not_claim_yesterday_missed_window test_already_ran_yesterday_does_not_refire_for_yesterday_window
The HH:MM parsing/normalisation/deduplication logic existed in two places:
- scheduler_wizard.py: inline loop producing list[str]
- daemon.py parse_daily_at_times: inline loop producing list[tuple]
Both shared the same strptime / strftime core but diverged in output type,
deduplication, and warning style — a textbook DRY violation.
Changes:
code_puppy/scheduler/time_utils.py (new)
parse_times_hhmm(raw, on_invalid=None) -> list[str]
- strips whitespace, ignores empty segments
- validates with strptime('%H:%M'), normalises via strftime('%H:%M')
- deduplicates preserving first-occurrence order
- calls on_invalid(entry) for each bad entry when provided,
silently skips otherwise
code_puppy/scheduler/daemon.py
parse_daily_at_times now wraps parse_times_hhmm with a
[Scheduler] Warning: callback and converts strings to (h, m) tuples.
Public interface and all existing tests unchanged.
code_puppy/plugins/scheduler/scheduler_wizard.py
Inline 10-line loop replaced with a single parse_times_hhmm call
using a local _warn_invalid callback for the emoji-prefixed output.
'from datetime import datetime' import removed (no longer needed).
tests/scheduler/test_time_utils.py (new, 23 tests)
Covers: single/multiple times, order preservation, normalisation,
deduplication, whitespace/empty-segment handling, invalid entries,
on_invalid callback behaviour (called per bad entry, not for empty
segments, not called for valid entries, None default is safe).
101 tests total, all green.
t-granlund
pushed a commit
to t-granlund/code_puppy-1
that referenced
this pull request
Mar 3, 2026
…nberger#188) * fix: fix summarization compaction strategy - multiple issues Root causes and fixes: 1. **Executor shutdown (DBOS crash)**: asyncio.run() shuts down the default executor, breaking DBOS's asyncio.to_thread() calls. Fix: Use loop.run_until_complete() in a dedicated thread instead. 2. **DBOSAgent async conflict**: Wrapping summarization agent in DBOSAgent caused 'event loop already running' errors. Fix: Keep summarization as a plain Agent (doesn't need durability). 3. **Orphaned tool calls**: Messages with tool_use but no matching tool_result were sent to the LLM, causing API 400 errors. Fix: Prune orphaned tool calls before sending to summarization. 4. **Split boundary breaks pairs**: Message splitting could separate tool_use from its tool_result between summarize/protected groups. Fix: Added _find_safe_split_index() to keep pairs together. 5. **Empty error messages**: Exception handler showed 'Summarization failed during compaction:' with nothing after the colon. Fix: Added SummarizationError with type/message details. Fixes mpfaffenberger#215 * revert: remove test changes from mpfaffenberger#215 fix - production code only * chore: update tests for new summarization impl, fix ruff lint/format - Update test mocks to match thread pool pattern (no more asyncio.run) - Remove get_use_dbos references (DBOS no longer used for summarization) - Add SummarizationError assertions in error propagation tests - Remove unused get_use_dbos import (ruff F401) - Apply ruff formatting * refactor: remove dead code _run_async_safely and _cancel_all_tasks These functions were never called - _run_in_thread already handles its own event loop creation and task cancellation inline. * fix: add missing dataclasses import and move set_message_history outside loop - Added 'import dataclasses' needed by dataclasses.replace() call - Moved self.set_message_history() outside the for loop so history is updated once after filtering completes, not on every iteration --------- Co-authored-by: n0h0123 <nathan.hicks0@walmart.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
2 participants
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.
What
Adds a new
daily_atschedule type that fires a scheduled task at one or morespecific wall-clock times each day (e.g.
09:00, or09:00,17:30).Why
The existing schedule types (
interval,hourly,daily,cron) don't letusers say "run this at 9am". The
daily_attype fills that gap with a simpleHH:MMformat — no cron syntax required.Changes
code_puppy/scheduler/daemon.pyparse_daily_at_times(schedule_value)— parses a comma-separated stringof
HH:MMtimes into(hour, minute)tuples. Invalid entries are skippedwith a warning rather than crashing the daemon.
should_run_task()— newdaily_atbranch fires if any target time haspassed today and
last_runis before that target. Restart-safe: if the daemonwas down at
09:00and restarts at09:15, it still fires.code_puppy/scheduler/config.pyschedule_typefield comment to documentdaily_atas a valid value.code_puppy/plugins/scheduler/scheduler_wizard.py"Daily"menu option with"Daily at specific time(s)...".TextInputMenu, validates eachHH:MMentry, stripsinvalid ones with a warning, and aborts if none are valid.
"daily at 09:00,17:30"instead of the raw internaltype/value string.
Tests
tests/scheduler/test_daily_at.py(new, 31 tests)Covers
parse_daily_at_times()and all branches ofshould_run_task()forthe
daily_attype. All wall-clock comparisons use a pinnedNOWconstant sotests are never flaky.
last_runlogictests/plugins/test_scheduler_wizard.py(6 new tests + 1 fix)test_daily_at_single_timeHH:MM→schedule_type="daily_at"test_daily_at_multiple_timesschedule_valuetest_daily_at_cancel_time_inputNonefromTextInputMenu→ wizard returnsNonetest_daily_at_all_invalid_timesNonetest_daily_at_strips_invalid_timestest_daily_at_summary_display"daily at HH:MM"appears in stdout; raw"daily_at"string does not leaktest_wizard_code_puppy_first(fix)"Daily"which was removed fromschedule_map; updated to"Every hour"Summary by CodeRabbit
New Features
Enhancements
Documentation
Tests