KofTwentyTwo/nuncio
2
0 Fork 0
Code Issues 73 Pull Requests 12 Actions Packages Projects Releases Wiki Activity

M1: Wire RRULE recurrence expansion into calendar event queries #186

New Issue
Closed
opened 2026-07-28 14:04:22 +00:00 by KofTwentyTwo · 1 comment
KofTwentyTwo commented 2026-07-28 14:04:22 +00:00 (Migrated from github.com)
Copy link
Copy source

Context

Story for milestone M1. Read docs/STORY-WORKFLOW.md first — it defines the process, gates, and patterns; this issue defines only the specific work.

crates/nuncio-cal/src/rrule.rs's RecurrenceEngine::expand_occurrences is a real, fully-tested RFC 5545 RRULE expansion engine (handles FREQ/INTERVAL/UNTIL/COUNT, caps output at MAX_EXPANSION_COUNT = 500, and correctly returns an error rather than a fabricated timestamp for an invalid rule) — but nothing in the codebase ever calls it outside its own unit tests. nuncio-store::db::DatabaseEngine stores each calendar event as exactly one row, including its raw rrule string (crates/nuncio-store/src/db.rs:918-942), and list_calendar_events (lines 949-996) filters by WHERE account_id = ? AND start_time <= ? AND end_time >= ? against that ONE stored row's own start/end — so a weekly-recurring event stored with its first occurrence's timestamps is only ever returned for query windows overlapping that first occurrence; every later occurrence is invisible. This story closes that gap: querying a window must materialize the recurring event's actual occurrences that fall inside it.

Goal

When a stored calendar event carries a non-empty rrule, querying an event window must return its real per-occurrence instances that overlap that window (via RecurrenceEngine::expand_occurrences), not just the single raw stored row. A non-recurring event's behavior must be unchanged. This must NOT silently drop or duplicate events: a recurring event whose master row happens to also overlap the window must not appear both as the raw row AND as an expanded occurrence. An unparseable rrule must surface as a genuine, visible error (or safely degrade to the single stored event, per RecurrenceEngine's own existing fallback behavior) — never a fabricated or duplicated event.

Files to touch / patterns to mirror

  • crates/nuncio-store/src/db.rs — add pub async fn list_recurring_calendar_events(&self, account_id: &str) -> Result<Vec<nuncio_core::model::CalendarEvent>, DatabaseError>, mirroring the shape of list_calendar_events (lines 949-996) but with WHERE account_id = ? AND rrule IS NOT NULL and no time-window filter (recurrence can validly start long before the query window and still produce occurrences inside it). Keep list_calendar_events itself unchanged.
  • crates/nunciod/src/grpc.rs — this is the layer that already depends on BOTH nuncio-store and nuncio-cal (nuncio-store itself does not and should not gain a dependency on nuncio-cal's RecurrenceEngine — keep expansion out of the storage layer). In CalendarGrpcService::list_events (added by M1-S1), after fetching:
    • let all_in_window = db.list_calendar_events(&req.account_id, req.start_window, req.end_window).await?;
    • let recurring = db.list_recurring_calendar_events(&req.account_id).await?;
      compute the final event list as: non-recurring rows from all_in_window (e.rrule.is_none()), PLUS every occurrence produced by RecurrenceEngine::expand_occurrences(&event, req.start_window, req.end_window) for each event in recurring — this both fixes the missed-later-occurrences bug AND avoids double-counting a recurring master that also happened to land in all_in_window, since recurring rows are always sourced from recurring/expansion, never from all_in_window. Extract this merge as a small, pure, directly unit-testable helper function (e.g. fn expand_events_for_window(all_in_window: Vec<CalendarEvent>, recurring: Vec<CalendarEvent>, start_window: i64, end_window: i64) -> Vec<CalendarEvent>) so its logic is tested without a database or gRPC server.
    • RecurrenceEngine::expand_occurrences returns Result<Vec<CalendarEvent>, CalendarError> — an unparseable rrule must not silently vanish the event; either surface it as a per-event error (added to the RPC response is not supported by the current message shape — simplest correct behavior: fall back to including the raw stored master event when expansion errors, and tracing::warn! the parse failure, mirroring how FiltersGrpcService::reload_engine_from_store logs rather than fails the whole RPC for a single bad rule at crates/nunciod/src/grpc.rs:761-780) rather than making the entire ListEvents call fail for one malformed recurring event among many.
  • crates/nuncio-cal/src/rrule.rs — no change expected; only if you find expand_occurrences's existing behavior (e.g. its exceptions/EXDATE handling — check whether EXDATE is parsed at all today, since the RRULE string alone doesn't carry cancelled instances) needs a genuine fix to correctly support UNTIL/COUNT/exceptions per the Goal, do it here with new unit tests following the existing style (see weekly_recurring_event_expands_multiple_instances, crates/nuncio-cal/src/rrule.rs:102-111).
  • Tests: extend crates/nuncio-cal/src/rrule.rs's own unit tests if you change engine behavior; add new tests at the nunciod::grpc level (or wherever the merge helper lives) proving: (a) a weekly event's occurrence 4 weeks out is returned when queried for that later window even though the stored master's own timestamps are 4 weeks earlier; (b) a non-recurring event's behavior is unchanged; (c) a recurring event is never double-returned when its master row's own start/end also happens to overlap the query window; (d) an event with UNTIL correctly stops producing occurrences past the bound; (e) an unparseable rrule degrades to the raw event with a warning rather than failing the whole query.

Implementation steps

  1. Confirm CalendarGrpcService::list_events/GetEvent exist (from M1-S1 — see Dependencies below). If M1-S1 has not yet merged, still complete steps 2-3 below (they are independently valuable and testable against DatabaseEngine directly) and note in the PR that RPC-level wiring (step 4) is blocked pending M1-S1.
  2. Add DatabaseEngine::list_recurring_calendar_events; test it directly (ephemeral DB, a mix of recurring/non-recurring rows).
  3. Write the pure expand_events_for_window merge helper (or equivalent) and its direct unit tests using nuncio_cal::RecurrenceEngine + fixture CalendarEvents — no DB or network needed for these.
  4. Wire the helper into CalendarGrpcService::list_events.
  5. Add/extend RecurrenceEngine tests only if a genuine correctness gap (e.g. EXDATE/exception handling) is found; do not touch it otherwise.
  6. Run cargo verify.

Acceptance criteria

  • DatabaseEngine::list_recurring_calendar_events added and tested.
  • Querying a window returns the real expanded occurrences of a recurring event that overlap it, including occurrences whose timestamps differ from the stored master row's own start_time/end_time.
  • A recurring event is never duplicated (raw row + expanded occurrence) in the same query result.
  • Non-recurring events are unaffected (byte-for-byte identical behavior to before this story).
  • UNTIL/COUNT-bounded recurrence correctly stops producing occurrences past the bound, proven by a test over a queried window that spans past the bound.
  • An unparseable rrule never crashes or fails the whole query and never fabricates a bogus instance; document and test the exact degrade-and-warn behavior chosen.
  • cargo verify green (fmt + clippy -D warnings all-targets + test-all), 0 ignored tests.

Definition of done & process

Follow docs/STORY-WORKFLOW.md: no fabricated data; comment convention (intent-only, no story/GH/phase refs in .rs/.proto); mock-only tests. Branch story/m1-rrule-recurrence off dev; PR to dev with the gate output pasted as evidence; the PO reviews before the issue is closed.

Dependencies / notes

  • Depends on M1-S1 for the RPC-level wiring step (CalendarGrpcService::list_events must exist to wire the merge helper into it). The store-layer method (list_recurring_calendar_events) and the pure merge helper's unit tests do not depend on M1-S1 and can be built/reviewed independently if the two stories overlap in time — just coordinate the final RPC-wiring commit with whichever of the two PRs lands second.
  • Not a [PROTO] story — no proto changes.
  • Size: M.

Added scope (deferred from the M1-S1 review of #185)

Since this story reworks CalendarGrpcService::list_events, also apply the calendar_id filter that #185 left as a documented no-op: today ListEvents accepts req.calendar_id but returns all of the account's in-window events regardless. Filter it — cheapest is a post-query events.retain(|e| req.calendar_id.is_empty() || e.calendar_id == req.calendar_id) (or a store-level filter if you're already touching list_calendar_events). Add a test asserting an event in a different calendar_id is excluded. This closes the honesty foot-gun (CLI --calendar defaults to "default", and the mock backend already filters by calendar_id, so mock-sync and store-read currently disagree).

## Context Story for milestone M1. **Read [`docs/STORY-WORKFLOW.md`](https://github.com/KofTwentyTwo/nuncio/blob/dev/docs/STORY-WORKFLOW.md) first** — it defines the process, gates, and patterns; this issue defines only the specific work. `crates/nuncio-cal/src/rrule.rs`'s `RecurrenceEngine::expand_occurrences` is a real, fully-tested RFC 5545 RRULE expansion engine (handles `FREQ`/`INTERVAL`/`UNTIL`/`COUNT`, caps output at `MAX_EXPANSION_COUNT` = 500, and correctly returns an error rather than a fabricated timestamp for an invalid rule) — but nothing in the codebase ever calls it outside its own unit tests. `nuncio-store::db::DatabaseEngine` stores each calendar event as exactly one row, including its raw `rrule` string (`crates/nuncio-store/src/db.rs:918-942`), and `list_calendar_events` (lines 949-996) filters by `WHERE account_id = ? AND start_time <= ? AND end_time >= ?` against that ONE stored row's own start/end — so a weekly-recurring event stored with its first occurrence's timestamps is only ever returned for query windows overlapping that first occurrence; every later occurrence is invisible. This story closes that gap: querying a window must materialize the recurring event's actual occurrences that fall inside it. ## Goal When a stored calendar event carries a non-empty `rrule`, querying an event window must return its real per-occurrence instances that overlap that window (via `RecurrenceEngine::expand_occurrences`), not just the single raw stored row. A non-recurring event's behavior must be unchanged. This must NOT silently drop or duplicate events: a recurring event whose master row happens to also overlap the window must not appear both as the raw row AND as an expanded occurrence. An unparseable `rrule` must surface as a genuine, visible error (or safely degrade to the single stored event, per `RecurrenceEngine`'s own existing fallback behavior) — never a fabricated or duplicated event. ## Files to touch / patterns to mirror - `crates/nuncio-store/src/db.rs` — add `pub async fn list_recurring_calendar_events(&self, account_id: &str) -> Result<Vec<nuncio_core::model::CalendarEvent>, DatabaseError>`, mirroring the shape of `list_calendar_events` (lines 949-996) but with `WHERE account_id = ? AND rrule IS NOT NULL` and **no** time-window filter (recurrence can validly start long before the query window and still produce occurrences inside it). Keep `list_calendar_events` itself unchanged. - `crates/nunciod/src/grpc.rs` — this is the layer that already depends on BOTH `nuncio-store` and `nuncio-cal` (`nuncio-store` itself does not and should not gain a dependency on `nuncio-cal`'s `RecurrenceEngine` — keep expansion out of the storage layer). In `CalendarGrpcService::list_events` (added by **M1-S1**), after fetching: - `let all_in_window = db.list_calendar_events(&req.account_id, req.start_window, req.end_window).await?;` - `let recurring = db.list_recurring_calendar_events(&req.account_id).await?;` compute the final event list as: non-recurring rows from `all_in_window` (`e.rrule.is_none()`), PLUS every occurrence produced by `RecurrenceEngine::expand_occurrences(&event, req.start_window, req.end_window)` for each event in `recurring` — this both fixes the missed-later-occurrences bug AND avoids double-counting a recurring master that also happened to land in `all_in_window`, since recurring rows are always sourced from `recurring`/expansion, never from `all_in_window`. Extract this merge as a small, pure, directly unit-testable helper function (e.g. `fn expand_events_for_window(all_in_window: Vec<CalendarEvent>, recurring: Vec<CalendarEvent>, start_window: i64, end_window: i64) -> Vec<CalendarEvent>`) so its logic is tested without a database or gRPC server. - `RecurrenceEngine::expand_occurrences` returns `Result<Vec<CalendarEvent>, CalendarError>` — an unparseable `rrule` must not silently vanish the event; either surface it as a per-event error (added to the RPC response is not supported by the current message shape — simplest correct behavior: fall back to including the raw stored master event when expansion errors, and `tracing::warn!` the parse failure, mirroring how `FiltersGrpcService::reload_engine_from_store` logs rather than fails the whole RPC for a single bad rule at `crates/nunciod/src/grpc.rs:761-780`) rather than making the entire `ListEvents` call fail for one malformed recurring event among many. - `crates/nuncio-cal/src/rrule.rs` — no change expected; only if you find `expand_occurrences`'s existing behavior (e.g. its exceptions/`EXDATE` handling — check whether `EXDATE` is parsed at all today, since the RRULE string alone doesn't carry cancelled instances) needs a genuine fix to correctly support `UNTIL`/`COUNT`/exceptions per the Goal, do it here with new unit tests following the existing style (see `weekly_recurring_event_expands_multiple_instances`, `crates/nuncio-cal/src/rrule.rs:102-111`). - Tests: extend `crates/nuncio-cal/src/rrule.rs`'s own unit tests if you change engine behavior; add new tests at the `nunciod::grpc` level (or wherever the merge helper lives) proving: (a) a weekly event's occurrence 4 weeks out is returned when queried for that later window even though the stored master's own timestamps are 4 weeks earlier; (b) a non-recurring event's behavior is unchanged; (c) a recurring event is never double-returned when its master row's own start/end also happens to overlap the query window; (d) an event with `UNTIL` correctly stops producing occurrences past the bound; (e) an unparseable `rrule` degrades to the raw event with a warning rather than failing the whole query. ## Implementation steps 1. Confirm `CalendarGrpcService::list_events`/`GetEvent` exist (from **M1-S1** — see Dependencies below). If M1-S1 has not yet merged, still complete steps 2-3 below (they are independently valuable and testable against `DatabaseEngine` directly) and note in the PR that RPC-level wiring (step 4) is blocked pending M1-S1. 2. Add `DatabaseEngine::list_recurring_calendar_events`; test it directly (ephemeral DB, a mix of recurring/non-recurring rows). 3. Write the pure `expand_events_for_window` merge helper (or equivalent) and its direct unit tests using `nuncio_cal::RecurrenceEngine` + fixture `CalendarEvent`s — no DB or network needed for these. 4. Wire the helper into `CalendarGrpcService::list_events`. 5. Add/extend `RecurrenceEngine` tests only if a genuine correctness gap (e.g. `EXDATE`/exception handling) is found; do not touch it otherwise. 6. Run `cargo verify`. ## Acceptance criteria - [ ] `DatabaseEngine::list_recurring_calendar_events` added and tested. - [ ] Querying a window returns the real expanded occurrences of a recurring event that overlap it, including occurrences whose timestamps differ from the stored master row's own `start_time`/`end_time`. - [ ] A recurring event is never duplicated (raw row + expanded occurrence) in the same query result. - [ ] Non-recurring events are unaffected (byte-for-byte identical behavior to before this story). - [ ] `UNTIL`/`COUNT`-bounded recurrence correctly stops producing occurrences past the bound, proven by a test over a queried window that spans past the bound. - [ ] An unparseable `rrule` never crashes or fails the whole query and never fabricates a bogus instance; document and test the exact degrade-and-warn behavior chosen. - [ ] `cargo verify` green (fmt + clippy `-D warnings` all-targets + test-all), 0 ignored tests. ## Definition of done & process Follow [`docs/STORY-WORKFLOW.md`](https://github.com/KofTwentyTwo/nuncio/blob/dev/docs/STORY-WORKFLOW.md): no fabricated data; comment convention (intent-only, no story/GH/phase refs in `.rs`/`.proto`); mock-only tests. Branch `story/m1-rrule-recurrence` off `dev`; PR to `dev` with the gate output pasted as evidence; the PO reviews before the issue is closed. ## Dependencies / notes - **Depends on M1-S1** for the RPC-level wiring step (`CalendarGrpcService::list_events` must exist to wire the merge helper into it). The store-layer method (`list_recurring_calendar_events`) and the pure merge helper's unit tests do not depend on M1-S1 and can be built/reviewed independently if the two stories overlap in time — just coordinate the final RPC-wiring commit with whichever of the two PRs lands second. - Not a `[PROTO]` story — no proto changes. - Size: M. --- ## Added scope (deferred from the M1-S1 review of #185) Since this story reworks `CalendarGrpcService::list_events`, also apply the `calendar_id` filter that #185 left as a documented no-op: today `ListEvents` accepts `req.calendar_id` but returns all of the account's in-window events regardless. Filter it — cheapest is a post-query `events.retain(|e| req.calendar_id.is_empty() || e.calendar_id == req.calendar_id)` (or a store-level filter if you're already touching `list_calendar_events`). Add a test asserting an event in a different `calendar_id` is excluded. This closes the honesty foot-gun (CLI `--calendar` defaults to `"default"`, and the mock backend already filters by calendar_id, so mock-sync and store-read currently disagree).
KofTwentyTwo commented 2026-07-29 22:35:47 +00:00 (Migrated from github.com)
Copy link
Copy source

Delivered via PR #215 (merged to dev). Adversarial review PASS — no double-count, windowing correct (removed a real fabrication bug in expand_occurrences), termination bounded, calendar_id filter applied. F1 (stale comment) fixed pre-merge; F2/F3 (low windowing/predicate edges) tracked as a follow-up. M1 (Finish Calendar) complete.

Delivered via PR #215 (merged to dev). Adversarial review PASS — no double-count, windowing correct (removed a real fabrication bug in expand_occurrences), termination bounded, calendar_id filter applied. F1 (stale comment) fixed pre-merge; F2/F3 (low windowing/predicate edges) tracked as a follow-up. M1 (Finish Calendar) complete.
Sign in to join this conversation.
No Branch/Tag Specified
Labels
Clear labels
area-ci
CI / quality gates
 area-cli
nuncio-cli reference client
 area-docs
Documentation
 area-grpc
gRPC API layer
 area-mail
Mail protocols
 area-release
Release / packaging
 area-security
Security
 area-store
Storage / SQLite
 bug
Something isn't working
 deferred
Backlog, off the critical path; re-justify before scheduling
 dependencies
Pull requests that update a dependency file
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 github_actions
Pull requests that update GitHub Actions code
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 javascript
Pull requests that update javascript code
 phase-0
Roadmap Phase 0
 phase-1
Roadmap Phase 1
 phase-2
Roadmap Phase 2
 phase-3
Roadmap Phase 3
 phase-4
Roadmap Phase 4
 phase-5
Roadmap Phase 5
 question
Further information is requested
 rust
Pull requests that update rust code
 story
Roadmap story: full LLM-ready spec, done via engine+proto+CLI+E2E
 wontfix
This will not be worked on
No labels story
Milestone
No items
No Milestone M1: Finish Calendar (CalDAV)
Projects
Clear projects
No projects
Assignees
Clear assignees
forge-recovery (Forge Recovery) james.maes (James Maes)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: KofTwentyTwo/nuncio#186