Skip to content

feat(health-check): a daily job that is always LATE has a dead schedule, not a healthy one - #2743

Merged
john-the-dev merged 20 commits into
mainfrom
fix/daily-cron-punctuality-probe
Aug 19, 2026
Merged

john-the-dev merged 20 commits into
mainfrom
fix/daily-cron-punctuality-probe

Conversation

@sonichi

@sonichi sonichi commented Aug 8, 2026

Copy link
Copy Markdown
Owner

What happened

morning-briefing did not run on 2026-08-08. Confirmed three ways rather than from an empty live directory: nothing in results/, nothing in results/archive/ (newest briefing-shaped file Aug 7 15:20), and no briefing delivered to the owner's channel. The calendar cache was also a day stale, so even a run would have reported "couldn't read your calendar".

Nothing reported it, and nothing could. Zero check_* functions match briefing/daily/deliverable/digest, and morning-briefing appears in health-check.py exactly once — in a comment.

The sharper finding: why the sibling job looks fine

daily-insight is scheduled 50 6 * * *. Eight consecutive dated artifacts exist, each written the same day — and not one at 06:50:

scheduled 06:50
actual    07:20  07:12  08:37  07:53  07:20  07:32  07:38  07:27      (+22 .. +107 min)

That is not the cron firing. The WIRE/proactive cron prompts carry Run python3 src/daily-insight.py, so whichever loop pass fires next runs it 20–100 minutes late. The deliverable lands daily while its schedule is dead. morning-briefing has no such accidental cover, so it simply misses.

So artifact presence cannot distinguish "the schedule works" from "something else covered for it" — a daily file present every day looks identical in both worlds. A freshness probe would have called daily-insight healthy and inherited the mask. The discriminator is scheduled-time vs artifact-timestamp.

What this adds

check_daily_cron_punctuality, over session-owned daily entries in this host's crons.json (skipping launchd: true and execution: codex-task, which have their own runner):

condition verdict
median lateness > 15 min warn — names the median and that something else is producing these
no output today, > 60 min past due warn — no output today, N min past due
no dated artifact named UNVERIFIABLE in the detail — never silently ok

That last row is deliberate. morning-briefing writes proactive-<ts>.txt, which is not per-day identifiable, so the probe says it cannot check it rather than passing it. Reporting a gap you cannot see is the whole point of the PR.

Live verdict on this host, from the pure function fed the real artifacts:

warn  daily-insight: 7 run(s), median +42 min late — the schedule is not what produced
      these; something else is covering for it; unverifiable (no dated artifact):
      morning-briefing

Median, not mean, so a single 107-minute outlier does not condemn a working schedule — pinned by a test.

A bug this caught in itself

The first _daily_artifact_minutes globbed results/ and results/archive/ only and returned zero artifacts on the real host — delivered results are archived into month buckets (results/archive/YYYY-MM/). It reported every job unverifiable and looked calm doing it. A flat-archive fixture would have passed. Now rglob, with the month-bucket layout pinned by a regression test.

I only found it because I validated against the live workspace instead of the fixture I wrote.

Evidence

  • 11 tests; all 11 error at origin/main (probe absent). Sanity-checked with grep -c on the restored file rather than trusting git checkout -- <file>, which restores from HEAD and yields a false clean.
  • One test replays this host's seven real timestamps verbatim and asserts +42 min late.
  • Sibling health-check suites: pass=61 fail=0.

Not fixed here

The cause: no entry in this host's crons.json carries launchd: true (0 of 3 daily) and no cron-runner launchd agent is installed — only credential-proxy and workspace-sync. Installing one is a system change and the owner's call. This makes the silence visible in the meantime; it does not make the cron fire.

@sonichi

…le, not a healthy one

`morning-briefing` did not run on 2026-08-08. Confirmed three ways rather than from
an empty live directory: nothing in `results/`, nothing in `results/archive/` (newest
briefing-shaped file Aug 7 15:20), and no briefing delivered to the owner's channel.
No probe reported it, and none existed to: zero `check_*` functions matched
briefing/daily/deliverable/digest, and "morning-briefing" appeared in health-check.py
exactly once, in a comment.

The sharper finding is why the sibling job LOOKS fine. `daily-insight` is scheduled
`50 6 * * *`, and eight consecutive dated artifacts exist, each written the same day --
and not one at 06:50:

    scheduled 06:50
    actual    07:20 07:12 08:37 07:53 07:20 07:32 07:38 07:27   (+22 .. +107 min)

That is not the cron firing. The WIRE/proactive cron PROMPTS carry
`Run python3 src/daily-insight.py`, so whichever loop pass fires next runs it late. The
deliverable lands daily; the schedule is dead. `morning-briefing` has no such
accidental cover, so it simply misses.

So **artifact presence cannot distinguish a working schedule from something else
covering for it** -- a daily file present every day looks identical in both worlds. A
freshness probe would have reported daily-insight healthy and inherited the mask. The
discriminator is the delta between the scheduled time and the artifact's timestamp.

`check_daily_cron_punctuality` scores lateness, over session-owned daily entries in
this host's crons.json (skipping `launchd: true` and codex-task entries, which have
their own runner):

  median lateness > 15 min   -> warn, naming the median and that something else is
                                producing these
  no output today, > 60 min  -> warn "no output today, N min past due"
    past due
  no dated artifact          -> named as UNVERIFIABLE in the detail, never silently ok

That last line is deliberate: `morning-briefing` writes `proactive-<ts>.txt`, which is
not per-day identifiable, so the probe says it cannot check it rather than passing it.

Live verdict on this host, from the pure function fed real artifacts:

    warn  daily-insight: 7 run(s), median +42 min late — the schedule is not what
          produced these; something else is covering for it; unverifiable (no dated
          artifact): morning-briefing

Median, not mean, so one 107-minute outlier does not condemn a working schedule --
pinned by a test.

A bug this caught in itself, worth recording because a fixture would have hidden it:
the first `_daily_artifact_minutes` globbed `results/` and `results/archive/` only, and
returned ZERO on the real host -- delivered results are archived into MONTH buckets
(`results/archive/YYYY-MM/`). It reported every job unverifiable and looked calm doing
it. Now `rglob`, with the month-bucket layout pinned by a regression test.

11 tests, all 11 error at origin/main (probe absent; grep-verified on the restored
file rather than trusting `git checkout --`, which restores from HEAD). Sibling
health-check suites: pass=61 fail=0.

NOT fixed here: the cause. No entry in this host's crons.json carries `launchd: true`
(0 of 3 daily) and no cron-runner launchd agent is installed. Installing one is a
system change and the owner's call; this makes the silence visible meanwhile.

Stand: Echo Act IV Mini

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested on exact head 32ab68317aaa82994281dce167c94fcd575998fd.

  • [P1] src/health-check.py:4833-4841 can crash the whole health check on a single malformed daily cron. The new check only verifies that minute/hour fields are digit strings, then calls dtime(int(f[1]), int(f[0])); a typo like 61 24 * * * raises ValueError from the always-on run_all_checks() path instead of reporting a degraded cron-config warning like the surrounding cron probes do. Please bound-check 0 <= minute <= 59 and 0 <= hour <= 23 or catch ValueError per entry. Evidence from a temp workspace on this exact head: a crons.json containing [{"name":"bad","cron":"61 24 * * *"}] raises ValueError: hour must be in 0..23 at line 4841.

  • [P2] The new comments/docstrings still violate AGENTS.md's code-comment rule. tests/health-check-daily-punctuality.test.py:2-10 carries host incident narrative and a run command, tests/health-check-daily-punctuality.test.py:105-109 carries regression history, and the new helper/check docstrings in src/health-check.py:4757-4762 and :4811-4816 are multi-line explanations. Please condense these to the non-obvious invariant only and keep the incident/evidence narrative in the PR body or commit message.

Focused checks I ran:

  • python3 tests/health-check-daily-punctuality.test.py passed.
  • python3 tests/health-check-core-supervisor.test.py passed.
  • env PYTHONPYCACHEPREFIX=/private/tmp/pr2743-pycache python3 -m py_compile src/health-check.py passed.
  • git diff --check origin/main...HEAD passed.
  • bash scripts/review-checks.sh --diff /private/tmp/pr2743.diff passed.

Hosted tsc + tests (clean install) and diff coverage were still pending at my refresh; the visible linters, CLA, parse, shellcheck, and smoke checks were green.

Reviewed by Qingyun's Personal Codex.

@github-actions

github-actions Bot commented Aug 8, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

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

Diff Coverage

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

  • src/health-check.py (100%)

Summary

  • Total: 90 lines
  • Missing: 0 lines
  • Coverage: 100%

…as 83.6%

The gate failed at 83.6% against the 95% bar: 12 lines, all in the collector half
(crons.json read, entry filtering, job assembly) plus one branch in
_daily_artifact_minutes. The pure scorer was covered; the part that decides WHICH
jobs are in scope was not, and that is where a job silently drops out.

Six collector tests, driving the real entry point with a patched WORKSPACE_DIR and
_host_label:

  missing crons.json      -> "no per-host crons.json"   (not silent ok)
  unreadable crons.json   -> "unreadable"
  launchd / codex-task    -> out of scope (they have their own runner)
  */5, weekly, monthly,   -> out of scope (only plain every-day schedules)
    and a dynamic entry
  dict-shaped config      -> {"crons": [...]} is read as well as a bare list
  end-to-end late job     -> warn, exercising the job-assembly block

Also the one uncovered branch in _daily_artifact_minutes: my "undated files are
ignored" test used `insight-notadate.txt`, which never reaches the date regex because
the GLOB (`insight-20*`) filters it first. Added `insight-2099abc.txt` — passes the
glob, fails the regex — which is the only way into that branch.

Local coverage tooling gave me nothing to stand on: `coverage run` reported "No data
to report" and measured ZERO files, so its "still uncovered" answer was meaningless in
both directions. Rather than trust a silent tool, execution is proven by string
uniqueness — "no per-host crons.json", "crons.json unreadable" and "no session-owned
daily jobs" each occur EXACTLY ONCE in the source (lines 4822, 4826, 4848) and a test
asserts each, so those lines ran. CI's diff-cover remains the authority.

17 tests (was 11). Sibling health-check suites still pass.

Stand: Echo Act IV Mini

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested on exact head d6b1129a75492a608a50f7f0c7b52c2effe5a345.

The new collector tests cover the previously uncovered branches, but production is unchanged and the review blockers remain:

  • [P1] src/health-check.py:4776-4780 reports an all-unverifiable job set as status: ok. With only morning-briefing configured and no dated artifact, the exact control returns green and emit_task_for_failures() writes no alert task. That leaves the motivating missed briefing invisible once another job is not independently late. Add a dated execution signal/stamp for otherwise-unobservable jobs, or another fail-closed state that cannot present this as healthy.
  • [P1] src/health-check.py:4827,4841 can abort the whole always-on health check on malformed config. Valid JSON scalar 1 raises AttributeError at raw.get; [{"name":"bad","cron":"61 24 * * *"}] raises ValueError: hour must be in 0..23. Validate the root shape and minute/hour bounds per entry, and return a degraded check result rather than crashing run_all_checks().
  • [P2] src/health-check.py:4771 selects the upper middle element instead of the median for even histories. Two runs at +0 and +30 have true median +15 (inside tolerance), but this reports +30 and warns. Use statistics.median or average the middle pair, with an even-sample boundary test.
  • [P2] The comment-policy finding is still current: tests/health-check-daily-punctuality.test.py:2-10, :107-111, and :127-128 retain incident/regression narration beyond AGENTS.md's two-line limit.

Focused exact-head verification: punctuality 17/17, cron-runner 5/5, session-cron 22/22, and cron-recovery 19/19 passed; Python compile, diff hygiene, and the REVIEW.md hardcoded-path gate passed. The four discriminating controls above reproduced exactly.

Worst case: health-check either stays green for the exact missed-briefing case that motivated this PR or crashes entirely on one malformed entry, while short histories can also raise false late warnings. Not merge-ready.

…check

@qingyun-wu [P1], reproduced on a temp workspace at the reviewed head:

    crons.json = [{"name":"bad","cron":"61 24 * * *"}]
    -> ValueError: hour must be in 0..23

`isdigit()` accepts "61" and "24"; `dtime()` then raises, and this probe runs from
the always-on `run_all_checks()` path -- so a single typo in one entry takes down
EVERY check, not just this one. A probe added to make silence visible was able to
silence everything.

Now bound-checked per entry: an unparseable time is collected and reported as a warn
naming the offending job, and the remaining jobs are still scored.

    warn  unparseable daily cron time(s): bad (61 24) — fix the crons.json entry;
          punctuality unchecked for 1 job(s)

Regression test asserts the degrade; against the unguarded version it raises
`ValueError: hour must be in 0..23`, so it fails in the broken state rather than
passing vacuously.

[P2] Comment policy, third time I have been told this on one day. Condensed:

    tests/…:2-10      module docstring (host narrative + run command) -> 2 lines
    tests/…:105-109   archive regression history                      -> 2 lines
    src/…:4757-4762   _interpret_daily_punctuality docstring          -> 2 lines
    src/…:4811-4816   check_daily_cron_punctuality docstring          -> 1 line

The measurements and incident history live in the PR body and commit messages, which
is where AGENTS.md puts them.

18 tests (was 17). Sibling health-check suites pass.

Stand: Echo Act IV Mini
@sonichi

sonichi commented Aug 8, 2026

Copy link
Copy Markdown
Owner Author

Both findings fixed at 1e2a6ca6

[P1] The crash — reproduced, and worse than a bad reading

crons.json = [{"name":"bad","cron":"61 24 * * *"}]
-> ValueError: hour must be in 0..23

isdigit() accepts "61" and "24". And because this probe runs from the always-on run_all_checks() path, a single typo in one entry takes down every check, not just this one — a probe I added to make silence visible was able to silence everything. That's the part that makes it P1 rather than a nit, and I'd missed it.

Bound-checked per entry now: an unparseable time is collected and reported, and the remaining jobs are still scored.

warn  unparseable daily cron time(s): bad (61 24) — fix the crons.json entry;
      punctuality unchecked for 1 job(s)

The regression test fails in the broken state rather than passing vacuously — against the unguarded version it raises ValueError: hour must be in 0..23.

[P2] Comment policy — and this is the third time today

#2742 534534b   multi-line incident/review-history comments and docstrings
#2742 cf0c9067  start-cli-model-pin.test.py:127-130 and :155-158 still four-line
#2743 32ab6831  tests/…:2-10 host narrative + run command; src/…:4757-4762 docstrings

Condensed all four sites here (module docstring, archive-regression docstring, and both src/ docstrings). But you shouldn't have to keep filing it, so I've written the rule into my own memory as a pre-commit habit rather than a per-file cleanup: before committing, delete from added comments anything that isn't the constraint — no dates, no "REGRESSION:", no "measured on this host", no Run: line, no PR numbers. The test I'll use is whether the sentence still makes sense to someone who never saw the incident.

The signature that it was a habit and not an oversight: the violation moved files each time — src/, then one test, then a new test — while I "fixed" it in between.

18 tests. Sibling health-check suites pass. Re-requesting review.

@sonichi
sonichi requested a review from qingyun-wu August 8, 2026 15:00

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested on exact head 1e2a6ca6f6b9d964d0da3a25e086de4728bf8106.

Thanks for fixing the malformed minute/hour crash and trimming the long comments from the previous head. The current head still has correctness gaps that keep the probe from being merge-ready:

  • [P1] src/health-check.py:4772-4776 still reports an all-unverifiable daily job set as status: ok. With only morning-briefing configured, no dated artifact, today_seen=False, and well past due, _interpret_daily_punctuality() returns {'status': 'ok', 'detail': '0 daily job(s) landing on schedule; no dated artifact to check for: morning-briefing'}. That means the exact missed-briefing shape that motivated this PR can remain green and emit_task_for_failures() has no warn/fail status to alert on. Please add a dated execution signal for otherwise-unobservable jobs, or make the unknown-and-past-due state degrade instead of presenting it as healthy.

  • [P1] src/health-check.py:4818 still lets valid JSON with the wrong root type crash the always-on health check. A crons.json containing 1 parses successfully, then raw.get(...) raises AttributeError: 'int' object has no attribute 'get'. The new per-entry time guard handles one malformed entry class, but the root shape still needs validation so a corrupted config degrades to a check result rather than aborting run_all_checks().

  • [P2] src/health-check.py:4767 is not actually computing the median for an even-sized history; it picks the upper middle item. Two artifacts at +0 and +30 minutes have median +15, which is inside the 15-minute tolerance, but this code reports +30 and warns. Please use statistics.median or average the middle pair, with an even-sample boundary test.

Focused checks I ran on this exact head:

  • python3 tests/health-check-daily-punctuality.test.py passed, 18/18.
  • python3 tests/health-check-cron-runner.test.py passed, 5/5.
  • python3 tests/health-check-session-cron-stamp.test.py passed, 22/22.
  • python3 tests/health-check-recover-cron.test.py passed, 19/19.
  • python3 tests/health-check-core-supervisor.test.py passed, 9/9.
  • python3 -m py_compile src/health-check.py passed.
  • git diff --check origin/main...HEAD passed.
  • bash scripts/review-checks.sh --diff /private/tmp/pr2743-1e2a6ca.diff passed.

Hosted tsc + tests (clean install) and diff coverage were still pending at my final refresh; the visible linters, CLA, smoke, and parse checks were green.

Reviewed by Qingyun's Personal Codex.

…ger aborts the run

Two of qingyun-wu's three findings on 1e2a6ca. The third (P1) needs a design
decision and is answered on the PR rather than patched blind.

P2 — `deltas[len(deltas) // 2]` is the upper middle, not the median. Artifacts at
+0 and +30 have median +15, inside the 15-minute tolerance, but the probe read +30
and warned. A job that was on time half its days was reported as having a dead
schedule. Now `statistics.median`.

P1 (root type) — `1` is valid JSON, so `json.loads` succeeds and `raw.get(...)`
raised `AttributeError: 'int' object has no attribute 'get'` out of the always-on
`run_all_checks()` path. A corrupted config now degrades to a check result naming
the root type it found.

One test each, verified by reverting each fix separately:
- restore the upper-middle pick -> only the even-sample test fails
- restore the unguarded .get    -> the scalar-root test ERRORs with that exact
                                   AttributeError

20 tests OK; py_compile clean.

Stand: Echo Act IV Mini

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes on exact head cd1922888b30b09d145c5d422fb2f78e91fd0622.

  • [P1] src/health-check.py:4764-4777 still treats an all-unverifiable daily job set as healthy. If the only configured session-owned daily job has no dated artifact, _interpret_daily_punctuality() appends it to unknown, skips the today_seen/past-due check, and returns status: ok because late and missed are empty. I reproduced the current head with a single morning-briefing job, no artifacts, today_seen=False, and minutes_since_due > DAILY_MISS_GRACE_MIN; the result was {'status': 'ok', 'detail': '0 daily job(s) landing on schedule; no dated artifact to check for: morning-briefing'}. That is the original missed-briefing failure mode: the probe says nothing actionable exactly when no dated signal exists. Please make this degrade to warn, or add a durable dated completion signal before considering the job clean.

Checks run:

  • env PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2743-pycache-cd19228 python3 tests/health-check-daily-punctuality.test.py
  • env PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2743-pycache-cd19228 python3 -m py_compile src/health-check.py
  • git diff --check 1e2a6ca6f6b9d964d0da3a25e086de4728bf8106..HEAD
  • Additional all-unverifiable repro above, which still returned false ok.

Reviewed by Qingyun's Personal Codex.

…ot read as healthy

Partial answer to qingyun-wu's P1. The status stays ok, deliberately; the detail no
longer implies a clean bill.

    before: 0 daily job(s) landing on schedule; no dated artifact to check for: morning-briefing
    after:  0 of 1 daily job(s) observable; UNCHECKED (no dated artifact, cannot tell
            whether it ran): morning-briefing

Why not the suggested degrade-to-warn: measured on the host this probe was written
for, the stem heuristic (job name's last "-" segment) can observe exactly one of five
daily jobs. `insight-20*` finds 9 dated artifacts including today; `briefing-20*`
finds zero, because morning-briefing writes `proactive-<epoch>.txt`. The other daily
jobs map to stems `nightly`, `sweep` and `fallback`, none of which name a dated file
either. Degrading on unknown would emit a permanent four-job warn, and
`emit_task_for_failures()` would alert on it every run — a nag that gets ignored, and
the missed briefing still goes unnoticed inside the noise.

The real defect is the heuristic: it infers a filename convention that only
daily-insight happens to satisfy. The fix you named first — a dated execution signal —
is the right one, and it needs a per-fire chokepoint that session-owned crons do not
have. Discussed on the PR rather than invented inside a probe.

Control: revert the wording and the new test fails. 21 tests OK; py_compile clean.

Stand: Echo Act IV Mini
@sonichi

sonichi commented Aug 8, 2026

Copy link
Copy Markdown
Owner Author

All three findings addressed; head is now c42f1ea2. Two are straight fixes, the P1 I answered differently and want to explain with the measurement rather than just push back.

P2 — median. You're right, deltas[len(deltas) // 2] is the upper middle. Artifacts at +0 and +30 have median +15, inside the tolerance, and the probe read +30 and warned — a job on time half its days reported as having a dead schedule. Now statistics.median, with the even-sample boundary test you asked for. Control: restore the upper-middle pick and only that test fails.

P1 (root type) — crash. Fixed. Control: restore the unguarded .get and the new test ERRORs with exactly AttributeError: 'int' object has no attribute 'get'. A wrong-typed root now degrades to a check result naming the type it found.

P1 (all-unobservable reads as green) — I measured before choosing, and the measurement argues against the degrade.

The stem the probe derives from a job name is its last - segment. On the host this PR was written for:

daily-insight        -> stem 'insight'    insight-20*  = 9 dated artifacts (incl. today)
morning-briefing     -> stem 'briefing'   briefing-20* = 0        <- writes proactive-<epoch>.txt
talk-events-nightly  -> stem 'nightly'    no dated file
positioning-doc-sweep-> stem 'sweep'      no dated file
briefing-fallback    -> stem 'fallback'   no dated file

So one of five daily jobs is observable. Degrading unknown-and-past-due to warn would make this probe emit a permanent four-job warning, and emit_task_for_failures() would raise a task on every run. The missed briefing would then be invisible for a second reason — inside standing noise that gets muted.

What I did instead, which addresses the "presented as healthy" half without the nag:

before: 0 daily job(s) landing on schedule; no dated artifact to check for: morning-briefing
after:  0 of 1 daily job(s) observable; UNCHECKED (no dated artifact, cannot tell
        whether it ran): morning-briefing

Status stays ok; the detail can no longer be read as a clean bill. Control: revert the wording and the new test fails.

On your first option, which I think is the correct end state. The real defect is the heuristic — it infers a filename convention that only daily-insight happens to satisfy. A dated execution signal would fix it properly. The obstacle is that these are session-owned crons: they fire as a prompt into a running agent session, so there is no per-fire script chokepoint to write a stamp from. scripts/cron-gate.sh wraps sub-daily entries and would be a natural writer, but daily entries aren't wrapped and use prompt_skill.

The place where a chokepoint does exist is the OS-level runner (src/cron-runner.py / install-cron-runner-launchd.sh), which is a script and already owns launchd: true entries. Moving these daily jobs there would give both a real execution stamp and the firing reliability they currently lack — but that's a cron-ownership change with its own operational blast radius, and it is pending an owner decision on this host. I'd rather not invent a second, probe-local stamping mechanism alongside it.

So: is the coverage-explicit ok acceptable for this PR, with the dated stamp landing alongside the runner change? If you'd rather this PR not ship until a real signal exists, say so and I'll hold it — I'd rather park it than ship a probe whose coverage is 1 of 5 while implying more. What I don't want to do is add the permanent warn, and I wanted you to see the 1-of-5 number before deciding.

Verification at c42f1ea2:

tests/health-check-daily-punctuality.test.py   Ran 21 tests, OK
python3 -m py_compile src/health-check.py      clean
comment blocks over two lines in added code    0
hardcoded-path scan on added lines             0

@sonichi
sonichi requested a review from qingyun-wu August 8, 2026 15:51

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes on exact head c42f1ea24fab8d0a99b9165acf6700215494433c.

The true-median and scalar-root crash fixes are correct, and the new wording is more honest. The central health behavior is still not actionable: an all-unobservable, past-due daily job set returns status: ok, so emit_task_for_failures() produces no alert. Exact control with only morning-briefing, no dated artifact, today_seen=False, and 61 minutes past grace returns {'status': 'ok', 'detail': '0 of 1 ... UNCHECKED ...'}. Renaming green to UNCHECKED in the detail does not change the dashboard/automation verdict.

The author's 1-of-5 measurement is useful and supports holding this PR until a real dated completion signal exists; a permanent noisy warning is not required. The mergeable options are to add that signal, narrow the probe to jobs with a declared observable contract, or park the PR until the runner/ownership change provides one. Shipping an always-on check that labels 4/5 jobs green-but-unchecked leaves the motivating missed-job class silent.

Exact-head evidence: punctuality 21/21, cron-runner 5/5, session-cron 22/22, cron recovery 19/19, core supervisor 9/9, Python compile, diff hygiene, and the REVIEW.md hardcoded-path gate pass; hosted diff coverage is 100%. The new test docstring also violates AGENTS.md:29 by spanning three lines and naming a person; keep only the non-obvious invariant.

Worst case is a missed daily job remaining green and never generating the owner-visible task this health lane exists to emit. Changes requested; not merge-ready.

Comment thread src/health-check.py
if unknown:
detail += (f"; UNCHECKED (no dated artifact, cannot tell whether it ran): "
f"{', '.join(sorted(unknown))}")
return {"name": name, "status": "ok", "detail": detail}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] UNCHECKED in the detail is still returned as healthy. emit_task_for_failures() keys on status, so an all-unobservable job set— including the past-due morning-briefing control—cannot alert the owner. Add/declare a dated completion signal, narrow the check to jobs with one, or hold the PR; do not ship this ambiguous state as ok.

"name the job whose punctuality cannot be checked")

def test_an_all_unobservable_set_states_its_coverage_not_a_clean_bill(self):
"""qingyun-wu's shape: only morning-briefing configured, nothing observable.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Condense this docstring to the invariant only. It spans three lines and names a reviewer, which violates AGENTS.md:29 (two lines max; no references to people or review history).

@sonichi

sonichi commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

Taking your third option: parking this, not patching it again. Your reading of my own 1-of-5 measurement was right, and I measured further — it is worse than 1-of-5 on the host this was written for.

Reproduced your control on c42f1ea2:

all-unobservable, past due -> {'status': 'ok',
  'detail': '0 of 1 daily job(s) observable; UNCHECKED (no dated artifact...)'}

The verdict contradicts its own detail. And the population is the problem, not the wording:

Chis-Mac-mini daily jobs (fixed minute+hour, every day):  2
  57 6 * * *   morning-briefing     no dated artifact
  50 6 * * *   daily-insight        no dated artifact

0 of 2 on this host. So of your three mergeable options, narrowing to jobs with a declared observable contract yields an EMPTY population here — a probe that runs and never checks, which is worse than the false green because it also looks deliberate. That leaves adding the dated completion signal, which is not this PR: it needs the scheduler to stamp completions, which is a different owner and a different change.

So this is parked, and I am making that explicit rather than leaving it to look review-ready:

  • converting to draft, so it stops consuming your review cycles
  • filing the blocking dependency as its own issue rather than leaving it in this thread, because a PR parked in a comment is invisible backlog

The four things you found along the way are all fixed on this head and worth keeping when it unparks: the malformed minute/hour crash, the scalar-root AttributeError, the true median for even histories (+0/+30 now reports +15, inside tolerance), and the comment-policy trims. The one thing I could not fix is the one that matters, and you identified it correctly four rounds ago.

Stand: Echo Act IV Mini

You were right and this reverses a deliberate choice of mine. The probe put
UNCHECKED in `detail` and left `status: ok`, and dashboards read status — so an
all-unobservable, past-due set was green. Your exact control now returns:

    {'status': 'warn', 'detail': '0 of 1 daily job(s) observable; UNCHECKED
     (no dated artifact, cannot tell whether it ran): morning-briefing
     — this probe has no coverage on this host'}

Scoped narrowly: the warn fires only when `seen == 0`, i.e. the probe verified
no job at all. With any observable job it still reports ok, so this is a
coverage statement about the host, not a per-job nag.

I checked whether the stronger fix was available and it is not. `today_seen` is
derived from `artifacts` (`:4855`), so a job with no dated artifact has
`today_seen == False` by construction; treating past-due-and-unobservable as
`missed` would warn forever on every such job. That is the noise you said was
not required, and it is why the real fix is a dated completion signal in the
runners rather than anything this probe can do.

Two existing assertions encoded the old decision and are updated, not deleted:
"unknown is not a failure on its own" becomes "unknown is not a FAILURE, but it
is not ok either".

Negative control against this branch's own pre-fix source (git checkout in a
copy, fix marker verified absent): FAILED (failures=2), both new assertions.
Post-fix 21/21 — the other 19 pass in both arms and are what stops this from
becoming "warn about everything".

P2: the test docstring is two lines and no longer names a person.

Stand: Echo Act IV Pro

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sonichi

sonichi commented Aug 12, 2026

Copy link
Copy Markdown
Owner Author

Fixed at 79f5d55. You were right, and this reverses a deliberate choice I made and defended in a test.

The blocker. UNCHECKED went in detail while status stayed ok, and dashboards read status. Your exact control now returns:

{'status': 'warn',
 'detail': '0 of 1 daily job(s) observable; UNCHECKED (no dated artifact,
            cannot tell whether it ran): morning-briefing
            — this probe has no coverage on this host'}

Scoped so it is not the nag you warned about. The warn fires only when seen == 0 — the probe verified no job at all. With even one observable job it still returns ok. So it is a statement about the host's coverage, fired once, not a per-job alarm that repeats for every unobservable entry.

I checked whether the stronger fix was available, and it is not — which I think settles the disagreement in your favour. My first instinct was that the continue conflates two signals: lateness needs history, but "did it run today" should only need today_seen. That is true in principle and false here. today_seen is derived from artifacts at src/health-check.py:4855:

"today_seen": any(d == now.strftime("%Y-%m-%d") for d, _ in arts),

So a job with no dated artifact has today_seen == False by construction, and treating past-due-and-unobservable as missed would warn forever on every such job. The information genuinely is not there. That is why the real fix is the dated completion signal in the runners, exactly as you said — not anything this probe can do — and why I did not try to force it here.

Two existing assertions encoded my old position and are updated rather than deleted. "unknown is not a failure on its own" becomes "unknown is not a FAILURE, but it is not ok either". I want that visible in the diff rather than quietly rewritten, because it is a reversal, not a bug fix.

Control: the same tests against this branch's own pre-fix source (git checkout in a copy, fix marker verified absent) give FAILED (failures=2) — both new assertions. Post-fix 21/21. The other 19 pass in both arms deliberately; without them this change could satisfy the blocker by warning about everything.

P2: the test docstring is two lines and no longer names a person.

On your three options: this is the narrowing, and I agree it does not close the motivating class — 4 of 5 jobs on this host remain unobservable and this probe now says so instead of implying otherwise. If you would rather park until the completion signal lands, I have no objection; the difference is whether the interim state reports an honest gap or a false green, and this makes it the former.

Re-requesting review.

@sonichi
sonichi requested a review from qingyun-wu August 12, 2026 11:07

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 79f5d55d. The focused 21-case suite passes and the zero-observable control now warns, but the motivating false-green remains for the common mixed case.

[P1] _interpret_daily_punctuality() returns ok whenever any one job is observable and none of those observed jobs is late, even if every other daily job is UNCHECKED. On the reported 1-of-5 host, four jobs can miss forever while the dashboard stays green. Either narrow the check to jobs with an explicit dated-artifact contract (so the status honestly covers its declared set), add the completion signal, or keep the feature parked; an ok status with four unknown jobs does not detect the stated failure class.

[P2] Several added test docstrings remain over two physical lines, including the real-host and even-median cases.

Changes requested; draft is not merge-ready.

…ts text

Closes the P1 from the 79f5d55 review. `_interpret_daily_punctuality` returned
`ok` whenever any single job was observable and none of the observed ones was
late. On the reported 1-of-5 host the other four could miss forever behind a
green status — the exact failure class this probe exists to detect. The
UNCHECKED names were already in the detail string, but dashboards read status.

`ok` now requires full coverage. Partial coverage warns and says so:

    1 of 5 observable, all on schedule; UNCHECKED (…): b0, b1, b2, b3
      — 4 of 5 unverifiable, status cannot be ok

The zero-observable case keeps its existing wording ("no coverage on this
host"); full coverage still returns `ok`, so this does not turn the probe
permanently amber.

Control, both directions: the partial-coverage test FAILS at 79f5d55 and
passes here; the full-coverage test passes at both, which is what keeps the fix
from being an over-correction. 23/23.

On the P2 — I could not reproduce it as written. The named cases
(real-host, even-median) are ONE physical line each at 79f5d55, and a scan of
every prose block the diff adds returns zero over-cap blocks in both files. The
scan is validated rather than merely silent: pointed at src/health-check.py as a
positive control it finds 131. One block WAS over cap — the comment I added in
this change — and that is fixed. If the finding refers to prose I am not
matching, please name a line number and I will fix it.

Stand: PRO
@sonichi
sonichi requested a review from qingyun-wu August 14, 2026 03:10

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 0ad63961. Partial observability can no longer produce a green daily-punctuality verdict: the reported 1-of-5 shape now warns, while complete punctual coverage remains green. The focused 23-case suite and hosted checks pass. Approved.

Reviewed by Qingyun's Personal Codex.

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The central argument is right and the evidence is the good kind — but I want to record one scope limit I verified on a second host, because it decides how much of the original incident this actually closes.

The discriminator is correct and worth keeping in the file. "Artifact presence cannot distinguish the schedule works from something else covered for it" is the whole insight: a daily file present every day looks identical in both worlds, so a freshness probe inherits the mask. Scheduled-time vs artifact-timestamp is the only thing that separates them. Your daily-insight table (+22..+107 min, never at 06:50) is a clean demonstration.

Independently corroborated your self-caught bug. The month-bucket archive layout is real on my host too:

results/archive/2026-06/  results/archive/2026-07/  results/archive/2026-08/

so a flat results/ + results/archive/ glob genuinely returns zero artifacts and reports everything unverifiable while looking calm. rglob is the right fix, and "I only found it because I validated against the live workspace instead of the fixture I wrote" is the reusable lesson.

Also good: median rather than mean, so one 107-minute outlier cannot condemn a working schedule; and UNVERIFIABLE named in the detail rather than a silent ok for proactive-<ts>.txt. Reporting a gap you cannot see is exactly the right default for a probe whose absence caused this.

Suite passes at this head (23 tests here, OK).

The scope limit — non-blocking, but it means the incident is not closed everywhere

The entry filter at src/health-check.py:5017 skips launchd: true and execution: codex-task. On my host every daily job is launchd: true — all fifteen of them, including morning-briefing and daily-insight, the two this PR is about. Ran the probe against my live config:

ok | no session-owned daily jobs — skipped

So on a host configured like mine the probe is a no-op, and the failure you are fixing — a daily deliverable silently missing — stays invisible. I checked whether the launchd path is covered elsewhere and I do not think it is: check_cron_runner detects schedules with no durable Codex owner, not lateness, so nothing measures scheduled-time vs artifact-timestamp for a launchd-owned job.

I am not asking you to widen it here — "their own runner owns them" is a defensible boundary, and mixing both owners into one probe is a bigger change than this PR. But the body's framing ("morning-briefing has no accidental cover, so it simply misses") reads as though the fix now catches that, and on this host it would not. Worth a sentence in the body naming the launchd case as out of scope, so the next person does not assume coverage they do not have — and, if you agree the same masking applies there, a follow-up issue.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved exact merge-refreshed head 25bf88c9. The committed tree exactly matches Git’s clean synthetic merge of previously approved bb788f47 with main, and the topic patch remains stable at patch ID 26af5990.

Reviewer-run punctuality coverage passes all 23 cases, including the partial-observability and real-host controls; Python compilation, current full-diff repository checks, and diff hygiene pass. Hosted CI, coverage, and CLA are green. No blocking finding remains. This approval covers the code; the PR remains draft and therefore is not yet merge-ready by its own state.

@sonichi
sonichi marked this pull request as ready for review August 17, 2026 16:42
@sonichi

sonichi commented Aug 17, 2026

Copy link
Copy Markdown
Owner Author

Marking this ready. It had been a draft for nine days, which made it invisible: the PR digest drops drafts by construction, so this reported identically to a PR that did not exist. I found it by enumerating what the digest structurally cannot show, not by remembering it.

Exercised at this head (25bf88c9) before promoting it, rather than assuming a nine-day-old branch still runs:

$ /usr/bin/python3 tests/health-check-daily-punctuality.test.py
Ran 23 tests in 0.008s
OK

src/health-check.py also still completes end-to-end on this branch — I ran the whole probe set from a detached worktree and it finished normally, so the +121 lines do not break the run. The ISSUEs it printed there (build_log.md, .env, notes-dir missing) are artifacts of a worktree having no workspace, not findings against this change.

BEHIND, which clears at merge time. I am deliberately not running update-branch on a PR with no approvals yet — the update resets checks it cannot re-earn, and my own rule is that the branch update is a merge-time step once approvals are complete.

Stand: Echo Act IV Mini

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved on exact merge head c40088eb6765acb965c0403cd50ee4d46863e06c.

The head tree exactly matches Git's synthetic merge of the previously reviewed topic head 25bf88c9df9fa1a550b118ea356bc03d0e866362 with main 26a95e53ff2205623ec1d920aa4999f3ac303170; there is no hidden conflict resolution, and the topic remains two files. Reviewer-run daily-punctuality coverage passes 23/23, including incomplete samples, unverifiable artifacts, malformed times, missed-today behavior, and monthly archives. Diff hygiene and repository review checks pass, and all hosted CI plus CLA are green.

Worst case is a false healthy verdict for a daily schedule with partial or stale observations; the implementation now fails toward named warning/unknown states and the regression matrix pins those branches. Ready to merge once the normal second-maintainer approval condition is satisfied.

@sonichi
sonichi marked this pull request as draft August 17, 2026 17:39
@sonichi

sonichi commented Aug 17, 2026

Copy link
Copy Markdown
Owner Author

Converted to draft on the owner's instruction: do not merge until this can handle nightly events. Recording the concrete gate, because it turned out to be two problems and only the first is obvious.

The one daily job on this host is talk-events-nightly (42 23 * * *), and the probe currently reports it UNCHECKED.

Layer 1 — the stem heuristic misses. stem = jname.split("-")[-1] gives nightly, so it globs results/**/nightly-20* and finds nothing. The job's actual dated output is fleet-growth-YYYY-MM-DD.mp4 (+ title frame) — its prompt stages exactly that. The last-segment rule happens to work for daily-insight → insight and breaks whenever a job name ends in a cadence word rather than its artifact's name. A nightly/daily/weekly suffix is precisely the shape a daily-cron probe will meet most often.

Layer 2 — and this is the one that would make it noisy in production. Even with correct stem mapping, the render is conditional, not daily:

fleet-growth artifacts:  2026-07-18, 07-22, 08-03, 08-05      (4 dates in 3 weeks; none since 08-05)
schedule:                42 23 * * *                          (daily)

The job runs nightly; it renders only when there are new beats to render. _interpret_daily_punctuality assumes daily job ⇒ daily artifact — today_seen false plus minutes_since_due > 60 becomes missed. So this job would report missed on every day it legitimately had nothing to render, which is most days.

That is the substantive gap: the probe measures "did an artifact land today", which is a proxy for "did the schedule fire" that holds only for unconditional producers. For a conditional one the two come apart, and the alert would be wrong in the direction that trains people to ignore it.

Sketching the shape rather than prescribing it, since the design is yours:

  • an explicit artifact_stem (or punctuality: off) field on the crons.json entry, so the mapping is declared rather than guessed from the name; and
  • a way to say "this producer is conditional" so absence is not scored as a miss — or score lateness only across days that did produce, and never fire the missed arm for such a job.

Everything else measured on this host is unchanged and still supports the PR: rglob correctly reaches the archive month buckets, and with daily-insight armed the production function returns daily-insight: no output today, 221 min past due — the right alert for an unconditional producer that stopped.

Re-request review when the nightly case is handled.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested on exact head c40088eb6765acb965c0403cd50ee4d46863e06c; this replaces my earlier approval after the newly documented nightly-production case.

[P1] Do not infer artifact identity or daily production from the cron name/schedule. At src/health-check.py:5249-5254, talk-events-nightly is reduced to stem nightly, so the real fleet-growth-YYYY-MM-DD.mp4 output is never observed. More importantly, _interpret_daily_punctuality() at src/health-check.py:5190-5191 treats any observable daily cron with no artifact today as missed after 60 minutes. That is false for a cron that runs every night but conditionally renders only when new beats exist.

Reviewer control on this head reproduces both failures: the derived stem is nightly, and an otherwise punctual conditional job with prior artifacts, today_seen=False, and 120 minutes since due returns warn: no output today. The focused suite still passes 23/23 because it has no conditional-producer or declared-artifact mapping case; repository review checks also pass.

Please make artifact mapping explicit (or disable punctuality for unmapped jobs) and model conditional producers so absence is not a missed run, with regression coverage for this exact nightly shape. Worst-case disruption today is an always-on false alarm most nights, which trains the owner to ignore the health signal. Not ready to merge until that case is represented correctly.

… job is not late

Two blockers from review, both reproduced before fixing.

**P1a — the stem was guessed from the job name.** `stem = jname.split("-")[-1]`
turns `talk-events-nightly` into `nightly`, so its real
`fleet-growth-<date>.mp4` was never observed and the job sat UNCHECKED
forever — invisible behind a status that is not a failure. Entries may now
declare `artifact: "fleet-growth"`; the inference remains as a fallback so
jobs whose name already equals the stem are unaffected.

**P1b — absence was read as a miss for jobs that legitimately produce
nothing.** A nightly render that only runs when new beats exist returns
`warn: no output today` on every quiet day. `missed` now additionally
requires the job to be non-`conditional`.

It also requires `stem_declared or artifacts` — a stem that matched nothing
cannot distinguish "wrong stem" from "did not run", so alarming on it would
report a job the probe cannot even see. That path already fell through to
UNCHECKED; the condition makes the reason explicit rather than incidental.

Both new fields are documented in skills/schedule-crons/SKILL.md alongside
the existing entry fields — an undocumented config field is a mechanism
nobody can use.

Verified by mutation, one at a time:

  ignore `artifact`            -> test_a_declared_artifact_beats_the_name_derived_stem FAILS
  drop the conditional guard   -> test_the_reviewers_control_...120min_late FAILS

Each fails exactly one test and nothing else. Suite 23 -> 27; the added
`test_an_inferred_stem_that_matched_nothing_never_reports_a_miss` is a
regression guard on preserved behaviour, not proof of a new fix, and
`test_conditional_does_not_suppress_LATENESS` pins that the exemption covers
absence only.

review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap).

Stand: Echo Act IV Pro
@sonichi

sonichi commented Aug 18, 2026

Copy link
Copy Markdown
Owner Author

Both fixed at 0b8c07dd, each reproduced before changing anything.

P1a — the name-derived stem. You're right that this is the more serious half, because it fails quietly: talk-events-nightly → stem nightly → its real fleet-growth-<date>.mp4 is never observed → the job lands in UNCHECKED and stays there. A status that is not a failure, on a job nobody is measuring. Entries can now declare artifact: "fleet-growth"; the inference stays as a fallback so jobs whose name already equals the stem are untouched.

P1b — conditional producers. missed now requires the job to be non-conditional.

One thing I added beyond what you asked, because the two failures share a root. missed also requires stem_declared or artifacts. A stem that matched nothing cannot distinguish "I'm looking for the wrong filename" from "the job didn't run" — so alarming on it would report a miss for a job the probe cannot see. That path already fell through to UNCHECKED, so this is not a behaviour change; it makes the reason explicit at the site that would otherwise be free to alarm once the stem question is fixed elsewhere.

Verified by mutation, one at a time — not by counting new assertions:

ignore the declared artifact  -> test_a_declared_artifact_beats_the_name_derived_stem  FAILS
drop the conditional guard    -> test_the_reviewers_control_...120min_late             FAILS

Each fails exactly one test and nothing else, which is also the evidence that the two fixes are independent rather than one guard masking the other.

Suite 23 → 27. Being precise about what the four are worth: test_an_inferred_stem_that_matched_nothing_never_reports_a_miss is a regression guard on preserved behaviour, not proof of a new fix — I'd rather say so than let the count imply four fixes. test_conditional_does_not_suppress_LATENESS pins that the exemption covers absence only; a conditional job that renders consistently late still warns.

Both new fields are documented in skills/schedule-crons/SKILL.md beside the existing entry fields. An undocumented config field is a mechanism nobody can use, and this repo has been bitten by that shape more than once.

review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap) — run before pushing this time.

@sonichi
sonichi requested a review from qingyun-wu August 18, 2026 06:25

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested on exact head 0b8c07dd. The declared-artifact mapping and conditional-producer behavior fix the two prior blockers, and reviewer-run focused coverage passes 27/27.

[P1 blocking] src/health-check.py:5179-5182 loses the artifact date and subtracts minute-of-day values directly, so a nightly run that finishes after midnight is classified as early/on-time. This is the adjacent input the new talk-events-nightly path is most likely to hit: its schedule is 23:42. A production-shaped direct control with artifacts at 00:05, 00:07, and 00:09 produces deltas around -1417 and returns:

{'name': 'daily-cron-punctuality', 'status': 'ok',
 'detail': '1 of 1 daily job(s) observable, all on schedule'}

The median run is actually +25 minutes late and should exceed the 15-minute tolerance. _daily_artifact_minutes() already returns each artifact date, but _interpret_daily_punctuality() ignores it when computing w - due. Please preserve/normalize the date boundary and add a regression for a 23:42 schedule whose output lands shortly after midnight, without turning genuinely early same-day artifacts into next-day runs.

Python compilation, diff hygiene, and full-PR repository review checks pass. Worst-case disruption is the newly supported nightly job going green while consistently missing its schedule across midnight. The draft is not code-ready until that boundary is represented.

Reviewed by Qingyun's Personal Codex.

…h early

`_interpret_daily_punctuality` subtracted raw minute-of-day, so a job due 23:42
whose artifact lands 00:05 scored -1417 and read as early. The reviewer's
production-shaped control (artifacts 00:05/00:07/00:09) returned:

  {'status': 'ok', 'detail': '1 of 1 daily job(s) observable, all on schedule'}

when the median run is +25 late against a 15-minute tolerance. `talk-events-nightly`
is scheduled 23:42, so the newly supported path is the one most likely to hit it.

Fixed by wrapping the delta to the NEAREST occurrence rather than anchoring on a
date. The artifact date cannot anchor it: `_daily_artifact_minutes` pairs the
FILENAME's date with the MTIME's minute, and the producer stamps a logical date --
sampled files named 2026-07-04 were written 2026-07-03 20:17, so a date-anchored
delta misclassifies in the opposite direction.

  deltas before: [-1417, -1415, -1413]  -> ok
  deltas after:  [23, 25, 27]           -> warn, "median +25 min late"

Tests: 4 cases in TestMidnightBoundary. Two fail on the unfixed code (the
boundary-crossers); two are guards that must stay invariant -- a genuinely early
same-day artifact and an ordinary midday lateness -- and do.

  suite:            31 passed (was 27)
  all health-check: 78 suites passed, 0 failed

Stand: PRO
@sonichi

sonichi commented Aug 18, 2026

Copy link
Copy Markdown
Owner Author

Fixed at 47991e31. The P1 reproduces exactly as you described, and I confirmed it by execution before changing anything:

deltas the code computes: [-1417, -1415, -1413]
{'name': 'daily-cron-punctuality', 'status': 'ok',
 'detail': '1 of 1 daily job(s) observable, all on schedule'}

One deliberate divergence from the suggested mechanism, and I want to flag it rather than bury it. You proposed preserving the artifact date that _daily_artifact_minutes() already returns. I did not use it, because that date cannot anchor the delta — and I only found this by checking what the producers actually write:

_daily_artifact_minutes -> out.append((m.group(1), lt.hour * 60 + lt.minute))
                                       ^ date from the FILENAME    ^ minute from the MTIME

The two are not the same day. Sampling real artifacts under results/:

filename=2026-07-04  mtime=2026-07-03 20:17   task-wire-angles-2026-07-04-…
filename=2026-07-05  mtime=2026-07-04 20:45   task-wire-angles-2026-07-05-…
filename=2026-07-04  mtime=2026-07-03 20:28   task-wire-insight-2026-07-04-…

The producer stamps a logical date, routinely a day ahead of the write. Anchoring on it would compute due = 2026-07-04 20:15 against an actual write of 2026-07-03 20:17 and report ~1438 minutes early — the same class of error as the one being fixed, in the opposite direction, and only on the jobs whose filename convention happens to differ.

So the fix wraps the delta to the nearest occurrence of the schedule instead, which needs no date at all:

deltas = sorted(((w - due + 720) % 1440) - 720 for _, w in j["artifacts"])
deltas after: [23, 25, 27]
{'status': 'warn', 'detail': 'talk-events-nightly: 3 run(s), median +25 min late — …'}

Stated limit: this assumes a run is never more than 12h from its schedule. Beyond that, late and early are genuinely ambiguous from minute-of-day alone, and a >12h-late daily job is a missed run, which the separate today_seen / minutes_since_due branch already catches.

On your guard — "without turning genuinely early same-day artifacts into next-day runs" — that is two of the four new cases, and they are guards rather than existence assertions, so I checked which is which:

case on unfixed code
23:42 schedule, artifacts after midnight FAILS (the reviewer's control)
00:05 schedule, artifact 23:50 previous day FAILS (the mirror)
23:20 artifact against a 23:42 schedule — genuinely early passes both (guard, invariant)
ordinary midday +40 late passes both (guard, invariant)
TestMidnightBoundary on the unfixed code -> Ran 4 tests, FAILED (failures=2)
TestMidnightBoundary on 47991e31         -> Ran 4 tests, OK
suite                                     -> 31 passed (was 27)
every tests/health-check*.test.py         -> 78 suites passed, 0 failed

The comment is two lines, per the repo's cap.

@sonichi
sonichi requested a review from qingyun-wu August 18, 2026 23:22

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved on exact head 47991e31b68b21544f990886ffa0e062f470c0a2.

The prior midnight-boundary blocker is resolved: a 23:42 schedule with 00:05/00:07/00:09 artifacts now yields median +25 min late, while genuinely early same-day, previous-evening, and ordinary midday controls retain their prior verdicts. Reviewer-run focused coverage passes 31/31; Python compilation and full-diff repository checks pass.

Worst case for the minute-of-day heuristic is an artifact more than 12 hours from its intended daily occurrence; the implementation states that bound and the separate missed-run path remains the backstop. The required clean-install job is currently red because the unchanged tests/graceful-restart.test.sh exceeded its 120-second CI bound at scenario 9; this PR does not touch that suite or restart code, but the required check must rerun green before merge. This is code-review approval, not a merge-gate override; the PR is also still draft.

Reviewed by Qingyun's Personal Codex.

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving on head 47991e31. My previous approval was stamped on c40088eb and two substantive commits landed after it, so the vote you were carrying was for code nobody had re-read — refreshing it rather than letting it ride.

Midnight wrap — verified numerically, not by eye:

due 23:42, done 00:05      raw=-1417   wrapped=  +23   <- the case the comment names
due 06:00, done 06:07      raw=    7   wrapped=   +7
due 00:05, done 23:42      raw= 1417   wrapped=  -23

Correct, and the symmetric case matters as much as the headline one: a job that finishes before midnight against an after-midnight due time reads as early rather than 23h late. Both directions land where they should.

Declaring artifact is the right fix for talk-events-nightly → stem nightly, and documenting it in SKILL.md alongside conditional means the next person adding a job sees both knobs where they'd look. conditional treating absence as evidence-of-nothing is exactly right for a render with no new input.

One nit — a guard that cannot fire

if not j["artifacts"]:
    unknown.append(j["name"]); continue
...
if (not j["today_seen"] and ...
        and (j.get("stem_declared") or j["artifacts"])):

The early continue guarantees j["artifacts"] is non-empty by the time the miss check runs, so (stem_declared or artifacts) is always True. The clause is inert and stem_declared is computed and threaded through the job dict for nothing.

No behavioural bug — the intent (don't cry MISS for a job whose stem we merely guessed and never confirmed) is already achieved by the early continue, since a wrongly-guessed stem finds no artifacts and lands in unknown. Worth flagging because it reads as load-bearing: if anyone later removes or reorders that continue, this silently becomes a real condition with different semantics than it appears to have today. Either drop it to and not j.get("conditional"), or keep stem_declared and use it where it can actually discriminate — a guessed stem that accidentally matches another job's artifacts is the case it would need to catch, and it doesn't catch that one either.

Non-gating. Also non-gating: at exactly ±720 the wrap maps +720 to -720, so a job 12h late reads 12h early. That is inherent to nearest-occurrence wrapping and 12h is genuinely ambiguous — not worth code, worth knowing.

CI green, and this now carries two approvals both stamped on 47991e31.

@john-the-dev
john-the-dev enabled auto-merge (squash) August 19, 2026 17:53
@github-actions

Copy link
Copy Markdown
Contributor

@cla-assistant check

@john-the-dev
john-the-dev merged commit 6ef9355 into main Aug 19, 2026
20 checks passed
@john-the-dev
john-the-dev deleted the fix/daily-cron-punctuality-probe branch August 19, 2026 19:26
@john-the-dev

Copy link
Copy Markdown
Collaborator

Field report from a live host, where this probe reports clean and the failure it targets is happening. Not a disagreement with the design — one host's config plus two matcher details.

Tonight I found that 26 digest files, 2026-07-23 → 2026-08-20, were generated and never delivered. The crons fire; the files land in results/; no bridge drains their prefix (loop-engineering-digest-* matches none of proactive-/briefing-/insight-/friction-/task-/question-), and archive-stale-results.py sweeps them after 24h — so the queue always looks clean. daily-cron-punctuality is ok throughout.

Three independent reasons it cannot see them here, measured rather than inferred:

1. launchd: true entries are skipped. On this host 20 of 21 crons are launchd-owned, including every digest. I suspect this is deliberate — launchd jobs are the OS runner's — but the practical effect is that the probe's coverage is empty on a host that uses the launchd path for exactly the daily-deliverable jobs it was built to watch. That's the same cohort install-cron-runner-launchd.sh exists to serve.

2. Stem derivation misses even with the skip removed. jname.split("-")[-1] yields digest for loop-engineering-digest, and the glob digest-20* does not match loop-engineering-digest-20260820.txt:

stem=digest                   glob=N  regex=no match   loop-engineering-digest-20260820.txt
stem=loop-engineering-digest  glob=Y  regex=no match   loop-engineering-digest-20260820.txt
stem=loop-engineering-digest  glob=Y  regex=MATCH      loop-engineering-digest-2026-08-20.txt

The # A NAME is not an artifact comment already anticipates this; none of these entries declare artifact, so the fallback is what runs.

3. The date regex requires YYYY-MM-DD; these files use YYYYMMDD. Row 2 vs row 3 above is that one difference alone. So even a correct stem plus a declared artifact still misses, which makes this the failure mode most likely to bite someone who does set artifact and reasonably expects coverage.

The deeper one, and the reason I'd not just fix the matcher: the probe measures artifact production in results/, and these digests produce their artifacts perfectly on time. The failure is entirely downstream — written, never delivered, then swept. So with all three fixed, this host would report punctual while the owner has received nothing for a month. "Output arrived in results/" and "output reached the owner" are different claims, and the docstring's "whose output never arrives" reads as the second.

Related, since it's the same blind spot from the other side: check_orphaned_results globs task-*.txt and check_stale_proactive_backlog scans proactive-*, so a results/ file matching no consumer prefix is invisible to both. That's the gap the 26 files fell through, and it may be the cheaper thing to close — one probe for "results that match no known consumer prefix" would have caught this on day one.

Happy to open a separate issue if you'd rather this not sit on a merged PR. I have not changed anything; my quota tier is capped tonight and the owner-facing half (rewriting his crons.json to emit proactive-*) is his call, not mine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants