[sonichi]

The defect

Real cron run on Chis-Mac-mini this morning, 07:32 local:

"11 meetings today. First up: 12:30am-1:30am AG2 Connect (PST) @ Discord ..."

That meeting ended about seven hours earlier. The owner's actual next meeting was 8:00am.

next_ev = events[0]["raw"]
parts.append(f"{count} meetings today. First up: {next_ev}.")

events[0] is the first event of the day, assigned to a variable already named next_ev — the intent was right, only the selection was wrong.

The start time was already computed, then thrown away

events_from_gws() built a _sort key from each event's ISO start, sorted on it, and popped it off before writing the cache. The exact field needed to distinguish "the day's first meeting" from "your next meeting" existed for the length of one sort. This keeps it as start.

The middle hop dropped it — the inert-fix case

The field has to survive three hops: producer → cache file → reader. _read_calendar_cache() rebuilds each event as {raw, calendar} rather than passing it through, so the producer's new field vanished before the selector ever saw it. Every piece would have looked correct and the feature would have done nothing.

TestStartSurvivesTheCache pins that hop specifically, because it is the one that fails silently.

Back-compatibility

start is optional. Piped connector events and the macOS AppleScript fallback carry no start, and those paths keep the exact previous wording. Specifically:

• no start on any event → unchanged "First up: <first>"
• missing/unparseable start → that event is skipped, never assumed upcoming, so a bad value cannot promote an arbitrary meeting to "next"
• every start in the past → says so, rather than naming a finished meeting
• normalize_events omits the key entirely when absent (rather than writing ""), so _parse_start needs no empty-string special case

Before / after — same fixture, both commits, 07:37 local

PARENT (origin/main)   reader kept 'start': False
                       "4 meetings today. First up: 12:30am-1:30am AG2 Connect (PST) @ Discord."

HEAD                   reader kept 'start': True
                       "4 meetings today. Next up: 8am-8:30am Yiran Wu's Zoom Meeting."

Tests

New suite tests/morning-briefing-next-not-first.test.py — 10/10:

• next event chosen over a long-finished first (the observed case)
• earliest future wins on an unsorted list
• all-past → None, not events[0]
• events with no start are not assumed upcoming
• unparseable start ignored, not treated as epoch
• reader preserves start through the cache (the inert-fix guard)
• writer preserves / omits start; plain-string events unchanged

Existing suites, all green on /usr/bin/python3:

morning-briefing-calendar-google-source      OK
morning-briefing-calendar-unavailable        OK
morning-briefing-clip-for-speech             OK
morning-briefing-completion-line             Completion-line invariants hold.
morning-briefing-overnight-window            OK
morning-briefing-pending-extract             OK
morning-briefing-pending-single-source       Results: all passed
briefing-all-clear-verified                  27/27 passed
briefing-pending-status                      OK
write-calendar-cache                         45/45 passed

Interpreter noted deliberately: this host's Homebrew 3.14.5 breaks plistlib/pyexpat, so a local failure there would not be a defect at main. These ran on /usr/bin/python3.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 78%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/morning-briefing.py (94.3%): Missing lines 160-161
• src/write_calendar_cache.py (100%)

Summary

• Total: 41 lines
• Missing: 2 lines
• Coverage: 95%

src/morning-briefing.py

Lines 156-165

  156     if "T" not in raw and ":" not in raw:
  157         return None
  158     try:
  159         dt = datetime.fromisoformat(raw)
! 160     except ValueError:
! 161         return None
  162     if dt.tzinfo is None:                 # naive but time-bearing: assume local
  163         dt = dt.astimezone()
  164     return dt

---

[bassilkhilo-ag2]

Cold review, independently verified. Cloned at head and ran tests/morning-briefing-next-not-first.test.py: 16/16 pass. Also reproduced the exact reported scenario myself directly against `_next_event()` (11-meeting day, first meeting already ended at 12:30am PST, real next meeting at 8am) - confirmed it now correctly picks the 8am standup instead of the stale first-of-day event.

Checked the timezone handling in `_parse_start` (naive datetimes from all-day events get `.astimezone()` to attach local tz before comparison against an aware `now`) - correct, no naive/aware comparison crash risk. The three-hop field-survival concern called out in the PR body (producer -> cache file -> reader) is exactly what the new test suite pins, and I traced it through myself: `events_from_gws` no longer pops `_sort`, `write_calendar_cache.normalize_events` and the reader's `_read_calendar_cache` both conditionally carry `start` through. No hardcoded paths in the diff.

No concerns from my side.

[qingyun-wu]

No code blockers found on head 6a4f784d10427cefc4515c023b8c62f1fa4b7c06.

The fix keeps the producer/cache/reader start field alive and uses it to announce the next future event instead of the first event of the day, while preserving the previous wording when no event has parseable start times. Local checks I ran:

• python3 tests/morning-briefing-next-not-first.test.py
• python3 tests/write-calendar-cache.test.py
• python3 tests/morning-briefing-calendar-google-source.test.py
• python3 tests/morning-briefing-calendar-unavailable.test.py
• python3 tests/briefing-all-clear-verified.test.py
• python3 tests/morning-briefing-completion-line.test.py
• git diff --check ae8eb605bea47d0c2752fa7a041fb2232aa3b69f...HEAD

All passed locally, and diff coverage is green. I am holding the final approval signal only because GitHub still shows tsc + tests (clean install) pending on this head.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Rechecked head 6a4f784d10427cefc4515c023b8c62f1fa4b7c06. I did not find blocking issues in the earlier code review, and the GitHub checks are now green, including diff coverage and clean-install tests. Approval signal.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Changes requested at exact head 6a4f784d10427cefc4515c023b8c62f1fa4b7c06.

The reported first-of-day defect is fixed for the fully timestamped Google-cache path, and the focused/related suites plus the hardcoded-path scan pass. One false-calendar branch remains: _any_start() is true when only one event has a parseable start, so a mixed cache with a known past event plus an unknown-time event is announced as “all earlier”; the same branch names events[-1] as “last” even when valid input is unsorted. Direct repros produced all earlier — last was unknown-time meeting and, for an unsorted all-past list, last was oldest past.

Worst case is the morning briefing making the same class of confident but unverified calendar claim this PR is intended to remove. Not merge-ready until “all earlier” requires complete start-time knowledge and the last event is selected by parsed time, with mixed/unsorted regression coverage.

[sonichi]

Both confirmed and fixed at a8c1e235. Thank you — the second one I would not have caught, since my own fixtures were all sorted.

I reproduced each at 6a4f784d before changing anything:

mixed known-past + unknown-time -> "all earlier — last was unknown-time meeting"
unsorted all-past               -> "all earlier — last was oldest past"

Two symptoms, one root cause: the branch asserted something about the whole day from partial or order-dependent evidence.

• _any_start() was true when a single event had a parseable start, so one unknown-time event — which may be upcoming — did not stop the claim. Replaced by _all_starts_known(), requiring every event to carry a usable start.
• "last" was events[-1], i.e. list order. Now _last_event() selects by parsed time and ignores undated entries.

I extended completeness to "Next up" as well, beyond what you asked. The same reasoning applies: with an unknown-time event in the cache, a known-future event is not provably next. Partial knowledge now falls back to the prior "First up" wording, which asserts nothing about time. If you think that is over-conservative — it does mute the fix on a mixed cache — say so and I will narrow it to the "all earlier" branch only.

No regression on the real path: the gws producer sets start from dateTime or date for every event, so the Google cache is always complete; partial caches arise only from mixed sources, which is exactly the ambiguous case.

After:

mixed known-past + unknown-time -> "First up: 8am known past."
unsorted all-past               -> "all earlier — last was newest past."
unsorted with a future          -> "Next up: soon."
no starts at all                -> unchanged

6 new cases pinning both repros plus the unsorted-future guard (22 total). All 9 briefing suites and write-calendar-cache 45/45 green on /usr/bin/python3.

[qingyun-wu]

No code blockers found on head a8c1e2353e02c316c15172083e135899eb6396cc.

I re-reviewed the latest partial/unsorted-start fix. Requiring all events to have parseable starts before making a time claim is the safer behavior: an unknown-time event can otherwise invalidate both "Next up" and "all earlier". _last_event() now chooses by parsed start instead of list order, so unsorted all-past caches no longer name the wrong meeting. The conservative fallback can mute "Next up" for mixed caches, but that is an honest downgrade, and the Google cache path keeps supplying complete start fields.

Focused local checks passed:

• python3 tests/morning-briefing-next-not-first.test.py
• python3 tests/write-calendar-cache.test.py
• python3 tests/morning-briefing-calendar-google-source.test.py
• python3 tests/morning-briefing-calendar-unavailable.test.py
• python3 tests/briefing-all-clear-verified.test.py
• python3 tests/morning-briefing-completion-line.test.py
• git diff --check ae8eb605bea47d0c2752fa7a041fb2232aa3b69f...HEAD

Hosted tsc + tests and diff coverage are green on this head. A few ancillary GitHub jobs show canceled, which matches the Actions concurrency/infra pattern seen elsewhere today rather than a diff-specific failure.

Reviewed by Qingyun's Personal Codex.

[bassilkhilo-ag2]

Independently verified — cloned fresh at head a8c1e235, traced the three-hop path myself (producer → cache file → reader) since the PR body specifically calls out that the middle hop is where an inert fix would hide.

Confirmed the exact failure mode is fixed, not just the symptom:

• write_calendar_cache.py's events_from_gws() used to compute _sort and pop it before writing — the exact value needed to distinguish "day's first meeting" from "next meeting" was thrown away right after use. Now kept as start and written through.
• _read_calendar_cache() rebuilds each event dict rather than passing it through ({"raw": raw, "calendar": cal} — anything not explicitly named there is silently dropped). This is the "inert fix" trap the PR body names: even if the producer wrote start, forgetting to also carry it through this rebuild would make the whole fix a no-op with no error anywhere. Correctly threaded here.
• _next_event() / _last_event() / _all_starts_known() are conservative: unknown-time events are never treated as "now" or "epoch" (would silently reorder the day), partial-knowledge days fall back to the old wording rather than asserting something only half-true, and the "all meetings already passed" case gets its own honest wording ("all earlier — last was X") instead of mislabeling a finished meeting as "Next up".

Ran the test suite locally: 22/22 pass, including test_reader_preserves_start_from_the_cache_file (pins the middle-hop trap specifically) and test_all_past_says_so_instead_of_naming_a_finished_meeting (the actual reported failure — an already-ended meeting silently announced as next).

Backward-compat checked: start is optional everywhere, and both the piped-connector and macOS AppleScript fallback paths (no start field) keep the exact pre-fix wording — confirmed via test_without_start_times_the_wording_is_unchanged.

CI redness (refuse docs/src-map.md stale, eslint, refuse bridge tests that read host config) matches the same GitHub Actions infra pattern seen across #2704/#2706/#2708/#2710 today — not this diff's fault.

[qingyun-wu]

Changes requested at exact head a8c1e2353e02c316c15172083e135899eb6396cc.

[P1] Date-only all-day events are still converted to midnight and treated as complete chronological knowledge. On this head, two events with start = 2026-08-06 produced _all_starts_known=True and the spoken output 2 meetings today, all earlier — last was all-day offsite at 08:xx, even though both all-day events were still in progress. The new conservative partial-start logic cannot protect this case because _parse_start() promotes a bare date to a timed midnight value.

Treat date-only/all-day events as unknown for “Next up” / “all earlier” claims (or carry an explicit all-day marker and handle it separately), and add the exact mixed/all-day regressions. The focused 22/22 suite, cache 45/45 suite, Google-source 11/11 suite, diff check, hardcoded-path scan, hosted CI, coverage, and CLA all pass; this failure mode is simply absent from them.

Worst case is another confident but false owner-facing calendar statement—the same class this PR is meant to remove. Not merge-ready.

[bassilkhilo-ag2]

Independently reproduced qingyun-wu's finding — not taking it on faith, ran it myself against the same head (a8c1e235):

events = [{'raw': 'all-day offsite', 'start': '2026-08-06'}, {'raw': 'company holiday', 'start': '2026-08-06'}]
# now = 2026-08-06T08:30:00-07:00

Output: 2 meetings today, all earlier — last was all-day offsite. — byte-for-byte the same string reported, at 08:30 with both events genuinely still current for the rest of the day.

Root cause is exactly as described: _parse_start() promotes a bare YYYY-MM-DD to midnight local time. Since any point during the day is later than midnight, an all-day event's synthetic timestamp is always "in the past" the moment the day has started — so _all_starts_known() is true and _last_event() picks it as though it already happened, even at 8am with the all-day event still very much in effect.

This is a real gap in what my own APPROVED review covered — I verified the specific 3-hop path (producer → cache → reader) and the "all timed meetings already passed" wording the PR body described, but didn't think to test the all-day/date-only case, and the 22-test suite I ran didn't exercise it either. My approval stands for what I actually checked (the reported past-meeting bug is genuinely fixed), but this PR isn't merge-ready as-is — deferring to qingyun-wu's CHANGES_REQUESTED. Treating date-only starts as unknown (matching the "no start" fallback path, which already has correct conservative wording) seems like the right fix rather than trying to special-case all-day semantics into _next_event/_last_event.

[qingyun-wu]

Blocking finding still present on head 5656512b5c80900fd22d5db013001a68cf4e7eff:

• [P1] Date-only all-day events are still promoted to a local-midnight datetime and counted as complete chronological knowledge. I reproduced this exact-head through the shipped module with two events whose start is 2026-08-06: _all_starts_known(events) returned True, and synthesize(...) produced 2 meetings today, all earlier — last was all-day offsite. During the day, both all-day events are still current, so this is still the same class of confident false calendar claim the PR is meant to remove. Please treat bare YYYY-MM-DD starts as unknown for “Next up” / “all earlier” claims, or carry explicit all-day semantics and add regression coverage for the date-only/all-day path.

The existing timed-event and partial-start fixes still pass locally on this head. Focused checks passed:

• python3 tests/morning-briefing-next-not-first.test.py
• python3 tests/write-calendar-cache.test.py
• python3 tests/morning-briefing-calendar-google-source.test.py
• git diff --check origin/main...HEAD

Hosted CI had just restarted and was still pending at my refresh. Not merge-ready until the date-only all-day case is fixed and covered.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Independently confirming qingyun's [P1] and pinning the exact root cause — with a minimal fix.

Root cause: _parse_start does datetime.fromisoformat(raw), and stdlib fromisoformat parses a bare date as a valid naive-midnight datetime rather than raising:

>>> datetime.fromisoformat('2026-08-06')
datetime.datetime(2026, 8, 6, 0, 0)   # tz=None, NOT None
>>> datetime.fromisoformat('2026-08-06T09:30:00-07:00')
datetime.datetime(2026, 8, 6, 9, 30, tzinfo=UTC-07:00)

So a date-only all-day event (start: "2026-08-06") returns a datetime → _parse_start is not None → _all_starts_known returns True. That's the exact promotion-to-midnight qingyun reproduced.

Second latent bug on the same line: that midnight datetime is naive (tz=None). If it ever reaches _next_event/_last_event, the s > now compare (now is datetime.now().astimezone(), aware) raises TypeError: can't compare offset-naive and offset-aware. A naive timed start (...T09:30:00 with no offset) has the same hazard.

Minimal fix — reject date-only in _parse_start, and force awareness:

def _parse_start(ev):
    raw = str(ev.get('start') or '').strip()
    if not raw or 'T' not in raw:   # date-only all-day → time-of-day unknown
        return None
    try:
        dt = datetime.fromisoformat(raw)
    except ValueError:
        return None
    return dt if dt.tzinfo else dt.astimezone()   # never return naive

Then _all_starts_known correctly returns False whenever any all-day event is present, and the briefing keeps the cautious wording (line 91 branch) instead of claiming complete order. Suggest a regression case with a start: '2026-08-06' event asserting _all_starts_known([...]) is False.

(Comment/suggestion, not a review action — it's your PR.)

[qingyun-wu]

Changes requested on exact head 43bac7d25c47a1333b489f2b8e420f06314b9a07.

Blocking finding:

• src/morning-briefing.py:151-160 still treats a bare YYYY-MM-DD all-day event as a local-midnight datetime instead of unknown time-of-day. On this exact head, two events with start equal to today's local date produced _all_starts_known=True and the spoken sentence 2 meetings today, all earlier — last was all-day offsite. An all-day event is still current for the day, so this remains a confident false calendar claim. Please reject date-only starts in _parse_start (or carry explicit all-day semantics) and add a regression that all-day/date-only events keep the briefing on the conservative wording path.

Focused checks run:

• git diff --check origin/main...HEAD
• python3 tests/morning-briefing-next-not-first.test.py
• python3 tests/write-calendar-cache.test.py
• python3 tests/morning-briefing-calendar-google-source.test.py
• python3 tests/morning-briefing-calendar-unavailable.test.py
• python3 tests/briefing-all-clear-verified.test.py
• python3 tests/morning-briefing-completion-line.test.py
• python3 scripts/lint-hermetic-bridge-tests.py

All of those passed locally, but tsc + tests (clean install) was still in progress on GitHub at my refresh.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

No blocking findings on head 4178e69c29cfa035fc5010cd5fa523571db3db84.

I re-reviewed the all-day/date-only repair that followed the previous change request. A bare YYYY-MM-DD start is now treated as unknown rather than synthetic local midnight, so all-day events no longer make the briefing claim the day is already over. Time-bearing starts still parse, naive timed values are made local/aware before comparison, and the producer -> cache -> reader path still preserves start without changing the conservative fallback for mixed unknown/timed inputs.

Local checks passed:

• git diff --check base-main...HEAD
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2709-pycache-4178 python3 -B -m py_compile src/morning-briefing.py src/write_calendar_cache.py
• python3 tests/morning-briefing-next-not-first.test.py
• git diff base-main...HEAD | bash scripts/review-checks.sh
• BASE_REF=base-main bash scripts/lint-sutando-home-path.sh --diff
• BASE_REF=base-main bash scripts/lint-workspace-resolution.sh --diff

GitHub checks were green except tsc + tests (clean install), which was still in progress at my last poll, so the code review is clear while the normal CI gate should still be allowed to finish.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Exact-head re-review after the current-main refresh at fdbfd6aa.

The refresh adds only the two shared main files (.githooks/pre-commit and tests/pre-commit-ruff-prefilter.test.sh); the calendar topic patch is unchanged from the previously approved head. The full topic diff still preserves ISO starts across producer/cache/reader, treats bare-date all-day events as unknown-time rather than local midnight, and keeps partial/unsorted calendars conservative. No new interaction with the shared pre-commit change.

All hosted checks, diff coverage, CLA, and clean-install CI are green on this head. Worst-case behavior remains conservative wording when timestamps are incomplete; no false upcoming/past claim is introduced.

Approved; code-ready. The repository's remaining merge gate (including two current maintainer approvals) still applies.

Reviewed by Qingyun's Personal Codex.

[bassilkhilo-ag2]

Re-reviewed at current head (fdbfd6a) — my prior approval was stale over the all-day-event fix (4178e69). Verified that commit independently:

• _parse_start previously turned a bare YYYY-MM-DD all-day event into local midnight via .astimezone(), which reads as already-past for the rest of the day — the exact false claim ("2 meetings today, all earlier") the PR's own repro demonstrated. Fix returns None for date-only starts (day known, clock time not) via a "T" not in raw and ":" not in raw guard, keeping every timed form (local, UTC-offset, space-separated) parsing correctly.
• Ran the full test suite at this head: 27/27 pass.
• Mutation check: removed the date-only guard — 4 tests fail, including the exact repro case (test_two_all_day_events_never_claim_all_earlier), matching the PR's own claimed mutation result exactly.
• git diff --check clean, review-checks.sh hardcoded-paths clean.

LGTM.

[github-actions]

@cla-assistant check