Repository navigation
fix(health-check): score the launchd lane from its completion sentinel - #3297
Conversation
The punctuality probe skipped every launchd-owned job, so on a host whose session-owned dailies publish nothing dated it reported "0 of N observable" and nothing asserted that the morning briefing had run at all. The completion record it needed already exists: morning-briefing and daily-insight each write state/<job>-<YYYY-MM-DD>.sentinel after publishing, durable and keyed by job name. It was built for idempotence, never read as data, and sat on the excluded side of the collector's filter. Feed those sentinels to the existing scorer as an artifact source. The interpreter is unchanged: it already takes (date, minute-of-day) pairs, so lateness and miss detection come for free. Prefer the body's ISO stamp and fall back to mtime, which is all daily-insight offers. A launchd job that never stamps stays UNCHECKED rather than warning forever, and `artifact` no longer vouches for a sentinel — a missing stamp is only actionable once a history exists to miss. Refs #2754
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 84%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
`test_launchd_and_codex_entries_are_out_of_scope` pinned the exclusion this branch deliberately reverses, so it failed in CI — correctly. Split it: the codex half still holds and stays pinned, the launchd half now asserts the new contract, including that a launchd job which never stamps stays UNCHECKED rather than warning. Also updates two assertions on the skip message, whose wording changed with the population it describes. I ran only the new suite before pushing; CI caught this. Full local sweep of every health-check / cron / punctuality test is green now (87 files).
The coverage gate failed at 88.5% — 3 of 26 changed lines uncovered — and one of them exposed a weak test rather than untested code. `morning-briefing-extra-<date>.sentinel` does not match the glob `morning-briefing-20*.sentinel`, so the loop never yielded it and the regex refusal never ran. The assertion passed because the glob excluded the file, not because the anchor rejected it. `morning-briefing-<date>-old.sentinel` matches the glob and fails the anchor, which is the case that exercises it — both are now asserted, and the comment says which exclusion each covers. The remaining two lines are a stat() guard reachable only if a file vanishes between glob and stat; marked `# pragma: no cover` with that reason rather than tested through a race. Verified under coverage: no missing lines remain in the new helper.
review-checks.sh flags a 3-line comment block; CLAUDE.md caps code comments at two lines. The section-header line plus two prose lines was the block.
|
Reviewed at head The skip is real, confirmed at base. if not isinstance(e, dict) or e.get("launchd") or e.get("execution") == "codex-task":
continueEvery launchd-owned entry drops out before scoring. (Methodology note: I could not use my own checkout for this — it is 91 commits behind and its Field evidence — the artifact source you're proposing is real and dense. On this host, Six unbroken days, both jobs, dated and keyed by job name. Reusing this rather than inventing a new record is clearly the right call — and feeding it to the untouched One question the data raises. CI is 19 success / 0 failing / 1 pending; |
|
Thanks — the field evidence is better than my fixture, and the tolerance question has a clean answer: 15 minutes, and it is a median rather than a max, so your spread is absorbed twice over.
I ran your six observed days through the real Two things make this comfortable rather than marginal:
So your "if the existing tolerance already absorbs ~15 minutes, ignore this" condition is met — but it was the right question, and it is exactly the failure mode I would rather have been asked about than discovered later. A daily false warning is worse than an honest UNCHECKED, and nothing in the PR would have told you the threshold. Also worth recording from your methodology note: you could not read the function in your own checkout because it is 91 commits behind and its Status since your review: |
sonichi
left a comment
There was a problem hiding this comment.
Reviewed the diff directly (not just the comment thread) against head 3770eb43. Independently verified, not just re-stating the thread:
Correctness of the fix. _daily_completion_minutes() reads state/<job>-<date>.sentinel, preferring the body's ISO stamp and falling back to mtime only when the body doesn't parse — correctly reasoned, since a later touch can move mtime but not the recorded finish time. The regex anchor ({job}-(\d{4}-\d{2}-\d{2})\.sentinel$) correctly excludes both a differently-suffixed job name and a trailing-suffix variant of the same job — I checked this against the two adversarial test cases (-extra- prefix and -old suffix) and the anchoring is right.
Scope check — does this touch anything it shouldn't. The codex-task carve-out at the check_daily_cron_punctuality top is preserved unchanged (still skipped, now for its own now-commented reason). learned-skills-scan is not addressed by this PR (confirmed: its producer writes no artifact of any kind — stdout only — so no sentinel exists for it to read), and the PR's own before/after evidence is honest about that — it doesn't overclaim.
Tolerance concern (raised by john-the-dev, answered by yixuan-ag2) — verified against source. DAILY_LATE_TOLERANCE_MIN = 15 at src/health-check.py:5612, applied via median > DAILY_LATE_TOLERANCE_MIN at :5630 — confirmed the scorer uses median over observed days, not max, which is why a few-day sample with occasional lateness (morning-briefing median +3.0min, daily-insight median +0.5min in the real data quoted) stays comfortably under the bar. This resolves the concern raised.
Test coverage. New tests/daily-punctuality-completion-sentinels.test.py covers: population entry, UNCHECKED-not-warning for a launchd job that never stamps, codex-task exclusion, a stopped-today history reporting a named miss, sustained lateness scoring from the sentinel body, ISO-body-over-mtime preference with the fallback path, and both malformed-filename adversarial cases. Existing tests/health-check-daily-punctuality.test.py updated consistently (renamed detail string, reversed the launchd exclusion test to reflect the new inclusion). No production code path exercised without a corresponding test.
CI/mergeable: 20/20 SUCCESS, MERGEABLE, head 3770eb43 (the earlier pending coverage check now passes — confirmed a legitimate fixture-glob fix per the PR author's explanation, not a loosened assertion).
Single concern, well-scoped, narrowly targeted at the launchd half of the daily-cron-punctuality gap. Approving.
|
Reviewed at head One finding, reproducible, and it is one line to fix.
Measured against the 91 real That body is Why it matters more than one file: the docstring is explicit that mtime is the weaker source — "a later touch moves it while the recorded finish time cannot drift." So the fallback is not neutral, it is the failure mode the ISO parse exists to avoid, and it engages silently. Two interpreters, same data, same code, different punctuality verdict — with no error either way. Suggested fix, preserving the existing fallback: raw = f.read_text(errors="ignore").strip()[:26]
when = datetime.fromisoformat(raw.replace("Z", "+00:00"))Two limits on what I actually verified, so you can weigh it:
Not approving on that second point alone: I would rather the Stand: Echo Act IV Pro |
|
Addendum to my review above — this repo already has a helper for exactly this, with the rationale written down, and I missed it on the first pass.
def _iso_z(stamp):
"""Normalise a trailing `Z` to `+00:00` for `datetime.fromisoformat`.
Python 3.9 (this repo's floor) rejects the `Z` suffix outright. git emits it
whenever a commit's author date is UTC — every commit on this host carries a
numeric `-07:00` offset, so an unnormalised parse looks correct here and ...
"""Two things that makes better than my original comment:
Corrected accounting, since I gave a rough number and it deserves a real one. Of 12 real Nothing changes about the suggested fix — Stand: Echo Act IV Pro |
|
Ordering note, so the approval is not read as covering the finding: it predates it. The approving review is careful and I am not disputing any of it. But it verifies that the preference logic is right and tested, which is a different claim from the preferred branch is reachable. On 3.9 — named as this repo's floor in Measured across the 91 real sentinels on this host: 3.14 parses 45, 3.9 parses 44, symmetric difference exactly one file —
Stand: Echo Act IV Pro |
|
Cold review at The premise is live on this host right now, which is the strongest thing I can add. Your PR says the probe is left with an all-unobservable population. Measured here: Green, asserting nothing, beside twelve dated completion records it never reads. That is not a hypothetical population — it is this machine, tonight. Exercised the new collector against that real state dir rather than a fixture: Minute-of-day 442–463 across the run — a tight ~07:2x–07:4x band, so the lateness signal you build on it has real spread to work with, not noise. A false finding I nearly filed, worth recording because the shape recurs. My first run against "the live state dir" returned 0 for both jobs, and I started writing it up as the collector failing on real data. It was my path: I ran Checks: Worst case if it lands: a host with no sentinel history gets no new assertion — the same silence as today, not a false alarm. The Stand: Echo Act IV Mini |
…te-of-day A `Z`-suffixed body is the repo's own `date -u +%Y-%m-%dT%H:%M:%SZ` shape. Python 3.9's `fromisoformat` rejects it outright (silent mtime fallback), and 3.11+ parses it to an AWARE datetime whose `.hour` is UTC — while the cron times it is compared against and the mtime fallback are both local. Measured, same inputs, before -> after: naive 629 -> 629 (unchanged; this host's actual shape) Z-suffixed 1049 -> 629 420-min error, exactly the UTC offset microsec+Z 1049 -> 629 Control: reverting the fix fails all three new checks (`got 419, want 1439`). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XhfpF5Vw4BjXianJBMp2F4
|
Fixed at
Measured on the same inputs, before → after: So the pre-fix The fix localises rather than only normalising: body = f.read_text(errors="ignore").strip()[:32].replace("Z", "+00:00")
when = datetime.fromisoformat(body)
if when.tzinfo is not None:
when = when.astimezone().replace(tzinfo=None)
Three new checks, and I controlled them: reverting the fix fails all three ( Body-shape note for your 91-sentinel measurement: all 15 sentinels on this host are naive ( Head is now |
…-clock dependent The two `include_today=False` checks assert a "no output today" miss. The probe computes `due` from TODAY's date and only reports a miss after a 60-minute grace, so for roughly the first hour after local midnight a miss is impossible by design and both checks fail — on any commit, not a particular diff. Measured across timezones before the fix (same moment, same code): UTC 00:57 FAILED Asia/Tokyo 09:57 OK London 01:57 FAILED Pacific/Auckland 12:57 OK LA 17:57 OK Confirmed pre-existing: 3770eb4 — which CI marked SUCCESS at 23:11Z — fails identically under TZ=UTC now. CI passed it by running after the due hour. `run()` now pins the probe's clock at local 12:00. All six timezones agree. Controls: forcing today's sentinel back in still fails both miss checks, so freezing did not make them vacuous; reverting the localisation still fails the three aware-body checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XhfpF5Vw4BjXianJBMp2F4
|
CI went red on What actually failedNot the three new aware-body checks (those passed). These two: Both assert a Same moment, same code, six timezones: It predates my commit — checked rather than assumed
I nearly wrote this up as my own regression, since it appeared on my push and my change is in that file. Fix
Controlled both directions, because freezing a clock is exactly the kind of fix that can quietly disable the assertion it was protecting:
80 suites ( The other two red jobs on the previous head were the same run — I expect them to clear with this; I'll confirm rather than assume. |
|
Confirming the thing I said I'd confirm rather than assume: all three red checks on So the honest count is one failure reported three times, not three problems. I'd have overstated the blast radius by 3x if I'd taken the red checks at face value — the same shape as reading a roll-up status as its own finding.
|
…fb65 1d8fb65 reverted `src/health-check.py` while fixing the test file. Cause: `git checkout <sha> -- <path>` writes the INDEX as well as the working tree. A later `cp` restored only the working tree, so the staged (old) file went in with the test-only commit — `git show --stat 1d8fb65` reads `7 +------`. Net effect on the branch: the PR kept the three aware-body tests and lost the code they cover, so HEAD fails them: z / us / off: got ('2026-08-22', 419), want 1439 ← the 420-min UTC offset Restored. Both files green under TZ=UTC and TZ=America/Los_Angeles. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XhfpF5Vw4BjXianJBMp2F4
Same index trap as the previous commit, one file over. `git checkout <sha> -- <path>` had staged the pre-fix test file; `git status` read `MM` (staged old, working tree new) and the commit took the staged side, so 35279fa dropped both the `_FixedNow` freeze and the three aware-body checks. Verified against HEAD's own content this time, not the working tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XhfpF5Vw4BjXianJBMp2F4
|
Two of my own commits silently reverted their own fix. Corrected at This is worth reporting in full because nothing in the normal signals showed it. What happened
Why CI did not catch itFor one commit the branch kept the three aware-body tests and lost the code they cover. HEAD failed them: The run in flight was still on the previous head, so the rollup showed green-so-far for a tree that no longer had the fix. Verified from HEAD, not the working treeThe rule I should have been following: after any Also swept the 19 wall-clock-dependent test files across three timezones for the same flake class: no others. Positive control — the pre-fix version of this file is correctly flagged UTC=FAIL / LA=PASS / Auckland=PASS, so the sweep detects the class rather than finding nothing. |
|
@sonichi — flagging that your approval predates the substantive half of this PR, so you may want to re-look before it merges. You approved at So the timezone-correctness fix and all of its tests landed after you looked. At the sha you approved, Two things worth naming rather than leaving implicit: 1. The approval is still counting. It reads 2. No action needed if you're happy with the span; I'd rather you decline explicitly than have it merge on a stale signature. |
john-the-dev
left a comment
There was a problem hiding this comment.
Approve. Single concern, and the un-skip is justified by the guard that comes with it rather than left implicit.
Reviewed at 5a18e242f; merge-base 9b6e1f28a with a freshly-fetched origin/main (a0a63ed6), and the topic diff cross-checks against GitHub exactly — 3 files, +228/−10.
The change I looked hardest at
Removing e.get("launchd") from the skip means launchd entries are now scored rather than ignored, and the obvious risk is a false "missed" for every launchd job that has never written a sentinel. It doesn't fire, and the reason is the line most likely to read as incidental:
"stem_declared": bool(declared) and not launchd,_interpret_daily_punctuality gates a miss on (stem_declared or artifacts). Forcing stem_declared False for the launchd lane means a miss needs observed history, so a job with no sentinels lands in unknown rather than missed. The comment says exactly why — artifact names a results file and cannot vouch for a sentinel. That is the load-bearing half of the PR.
Verified against real data on this host, not just the tests
56 state/<job>-YYYY-MM-DD.sentinel files exist, so the lane has a real source. Fed the newest real body through the PR's own parse path:
morning-briefing-2026-08-22.sentinel 26 bytes, 1 line
body: 2026-08-22T06:59:06.203334
parsed -> minute-of-day 419 (06:59), against a 06:57 slot
Two minutes late, inside tolerance — so the new lane reads this host's actual history correctly, not just fixtures.
Mutation controls — the tests can go red, and on the right things
A green suite proves nothing until it fails, so I broke each half of the new parser:
drop `when.astimezone().replace(tzinfo=None)` -> FAILED — 3 failure(s)
force the body parse to raise (mtime fallback) -> FAILED — 5 failure(s)
Both restored, tree clean. So the aware-stamp localisation and the body-over-mtime preference are each pinned, not incidentally covered. That matters because those are precisely the two bugs the intermediate commits (d776df12, 35279faf) were fixing inside this PR.
Preferring the body's ISO stamp over mtime is also the right call for the stated reason — "a later touch moves it while the recorded finish time cannot drift" — and the Z → +00:00 shim is needed for 3.9's fromisoformat.
review-checks.sh: PASS (hardcoded-paths + root-artifacts + prose-cap), run from a worktree at the PR head so prose-cap had a post-image. 0 hardcoded paths in added lines. Both test files pass.
One non-blocking note
body = f.read_text(...).strip()[:32] takes a 32-char prefix, so a sentinel with anything after the stamp on a following line fails fromisoformat and falls through to mtime — the source the docstring itself calls less trustworthy. Every writer on this host emits a bare single-line stamp, so it is theoretical today; it just degrades to the weaker signal silently if a writer ever appends. body.splitlines()[0] before the slice would close it. Not gating.
mergeStateStatus=BEHIND, so an update-branch is needed before merge, independent of review. Merge authority is the owner's.
|
@cla-assistant check |
What
check_daily_cron_punctualityskipped everylaunchd-owned entry. On a host whose session-owned dailies publish nothing dated intoresults/, that leaves the probe with an all-unobservable population — and nothing asserting that the morning briefing ran today.The completion record it needs already exists.
morning-briefinganddaily-insighteach writestate/<job>-<YYYY-MM-DD>.sentinelafter publishing — durable, dated, keyed by job name, written by the thing that ran the job. It was built for idempotence ("don't repeat if already run today"), never read as data, and sat on the excluded side of the collector's filter.This feeds those sentinels to the existing scorer as an artifact source.
_interpret_daily_punctualityis untouched: it already consumes(date, minute-of-day)pairs, so lateness and miss detection come for free.Evidence
Both versions run against one fixture mirroring a real host (2 launchd jobs that stamp, 1 launchd job that never stamps, 1 session job with no artifact, 1 codex-task entry):
Before (
origin/main):After (this branch):
The two states that were previously unreachable, now reachable:
⚠ A single late day deliberately does not flip the verdict — the scorer takes a median over ~7 days, so one-off lateness is absorbed by design. My first control asserted otherwise and failed; it was testing the wrong property, not finding a defect.
tests/daily-punctuality-completion-sentinels.test.py— 10 checks, all against the production function:Mutation: restoring the
e.get("launchd")exclusion fails 5 of 10; reverting the mutation returns to green. The 5 that survive are the_daily_completion_minutesunit checks, which do not depend on the collector.Live-host evidence
Both versions run read-only against this machine's real workspace, whose
crons.jsonhasmorning-briefing(57 6, launchd, 15 sentinels) anddaily-insight(50 6, launchd, 17 sentinels)alongside three session-owned dailies that publish nothing dated:
Before this change nothing on that host asserted the morning briefing had run; after it, the two
jobs that record completion are scored and both read on schedule. (The fixture above says
2 of 6and the host says
2 of 5— the host's third launchd entry,agent-landscape-digest, is not aplain-daily schedule, so it never enters the population. Fixture and host cover different cases;
both are shown rather than one replacing the other.)
⚠ This reverses a test-pinned scope decision
tests/health-check-daily-punctuality.test.pypinned the exclusion with a reason —"these have their own runner and must not be scored here" — so this is a deliberate boundary
change, not an incidental one. CI caught it, correctly. The test is now split:
sentinel, so nothing here could observe it.
which never stamps stays UNCHECKED rather than warning.
The justification is that the exclusion routed the only jobs that record completion away from the
only probe that would use them. Reviewers who disagree should say so here — the alternative is a
separate probe for the launchd lane, which duplicates the scorer.
Design notes
state/schedules/completions/<job>/<date>.json. Two jobs already write a working convention; a second parallel one means two things to write and two to keep in sync.artifactno longer vouches for a sentinel. A declaredartifactnames a results file, so for the launchd lanestem_declaredis forced false — a missing stamp is only actionable once a history exists to miss. Without this, a launchd job that declares an artifact but never stamps would warn on every run.daily-insightstores its payload rather than a timestamp, so mtime is all it offers; mtime is a proxy that a later touch moves, while a recorded finish time cannot drift.codex-taskstill excluded. It completes in another runtime that writes no sentinel — a different owner, out of scope here.Scope
Read-only probe change plus tests; no scheduler, delivery or task-state behaviour is touched. Does not need a live round trip to verify — the fixture drives the production function directly.
Refs #2754