Repository navigation
fix(health-check): daily-punctuality discarded every compact-dated artifact - #3439
Conversation
…tifact
`_daily_artifact_minutes` globs `<stem>-20*` and then requires the filename to
match `\d{4}-\d{2}-\d{2}`. Live results use both spellings and the compact one
dominates — 217 files vs 41 on this host — so every `<stem>-YYYYMMDD` artifact
was globbed and then dropped.
Measured on the live results tree, before -> after:
loop-engineering-digest 0 -> 7
money-opportunities-daily 0 -> 7
agent-loop-video-ideas 0 -> 6
Now accepts both and normalises to ISO, because the caller does
strptime(newest, "%Y-%m-%d") and compares against now.strftime("%Y-%m-%d") —
returning a raw 20260826 would raise there rather than fix anything.
Scoped honestly: this does NOT change this host's "2 of 11 observable" verdict.
Every daily entry here is `launchd: true`, and that lane reads completion
sentinels instead of artifacts, so it never reaches this matcher. The bug is
real for any non-launchd daily job, and the launchd routing is a separate
question.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
yixuan-ag2
left a comment
There was a problem hiding this comment.
Approving. I verified the defect independently rather than re-running your numbers, on a different stem and a different host's corpus:
stem=task-wake-briefing globbed=7 old_regex=0 new_regex=7
The glob collected all seven, the old \d{4}-\d{2}-\d{2} matched none of them, so the probe asserted over an empty set and was green. Same shape you reported, arrived at from a corpus you never touched — which is the part that makes it worth having as a second data point rather than a confirmation.
One note on my own method: my first attempt used your three stems and returned globbed=0 for all three. That zero was my instrument, not your claim — those artifacts do not exist on this host. I only avoided reporting "could not reproduce" as a finding because a zero arriving that easily is the tell. I then checked whether any dated artifacts existed here (18: 7 compact, 4 hyphenated) and picked a stem by the property instead.
On the diff itself: the widened (\d{4})-?(\d{2})-?(\d{2}) also accepts the mixed spellings 2026-0826 and 202608-26. Both normalise to the same ISO date through "-".join(m.groups()), so it is harmless — worth knowing, not worth tightening.
test_a_non_date_suffix_is_still_rejected is the assertion that earns this. Widening a matcher is exactly where it degenerates into a prefix match on the stem, and that test is the one that can fail if it does. I ran the same two probes plus a real compact date: only the date matches.
The scoping paragraph reads correctly to me — this changes which artifacts the probe sees, not what it considers punctual, so a job whose artifacts were previously all discarded can now legitimately report late where it used to report nothing.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 85%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
|
Reviewed at head Bug confirmed on Fails in the broken state, passes at head, and the compact dates come back normalised to ISO so the sort and the downstream One finding, non-blocking, about the rationale rather than the code. The added comment states that both spellings are in live use "and the compact one dominates". That does not reproduce on my host. Counting dated artifacts under Scoped honestly: that is one host's Capability vs configuration, since it changes who this helps. With zero compact-dated artifacts here, this is a no-op on this host today; its value is on hosts that write them. Worth stating so nobody reads the merge as fixing a punctuality gap they are currently seeing. The new Stand: Echo Act IV Mini |
qingyun-wu
left a comment
There was a problem hiding this comment.
Exact-head review at baf7051: no blocking findings. Compact and hyphenated artifact dates normalize to the caller's ISO contract, unrelated suffixes remain excluded, and the focused daily-punctuality suite passes locally. Diff checks are clean.
Reviewed by Qingyun's Personal Codex.
keweichen
left a comment
There was a problem hiding this comment.
APPROVE — exact head baf70516de018ee061d0c58c250d496318aa5bb8 is ready to merge, subject to the normal exact-head CI, CLA, and two-maintainer gates.
The merge-base and HEAD parent are both 21d6915917f748e7c5bdd140187a5ffa0f30bec4; this is one topic commit, so no parent stack layer applies.
I confirmed the defect on that parent by loading its _daily_artifact_minutes implementation and running the five added spelling tests against it:
parent: Ran 5 tests — FAILED (failures=2, errors=1)
HEAD: Ran 46 tests — OK
A collector-level fixture exercised the actual caller, not only the regex:
parent: warn | 0 of 1 daily job(s) observable; UNCHECKED ...
HEAD: ok | 1 of 1 daily job(s) observable, all on schedule
src/health-check.py:5895-5899 normalizes either spelling to ISO before the unchanged strptime and today_seen contracts at src/health-check.py:5994-6005. I traced the sole production caller and the sibling completion-sentinel lane at src/health-check.py:5983-5991; launchd behavior remains unchanged. Hyphenated artifacts and non-date controls remain covered.
Local checks:
python3 tests/health-check-daily-punctuality.test.py: 46/46 OKpython3 tests/daily-punctuality-completion-sentinels.test.py: OK, 0 failuresscripts/review-checks.sh: PASSgit diff --check: clean- No host-specific paths were added. A synthetic
/Users/reviewer/privatecontrol made the same scanner fail with one violation, confirming the negative result is meaningful.
Worst-case disruption is a misleading health status if a compact-looking file shares the configured stem but is not genuine job output. The blast radius is read-only and limited to this probe: exact-stem/date-shape filtering remains in place, and no schedules, files, processes, or persisted formats are changed. For genuine compact artifacts, changing UNCHECKED to observed is the intended correction.
Reviewed by Kewei's Codex automation.
) * fix(health-check): daily-punctuality discarded every compact-dated artifact `_daily_artifact_minutes` globs `<stem>-20*` and then requires the filename to match `\d{4}-\d{2}-\d{2}`. Live results use both spellings and the compact one dominates — 217 files vs 41 on this host — so every `<stem>-YYYYMMDD` artifact was globbed and then dropped. Measured on the live results tree, before -> after: loop-engineering-digest 0 -> 7 money-opportunities-daily 0 -> 7 agent-loop-video-ideas 0 -> 6 Now accepts both and normalises to ISO, because the caller does strptime(newest, "%Y-%m-%d") and compares against now.strftime("%Y-%m-%d") — returning a raw 20260826 would raise there rather than fix anything. Scoped honestly: this does NOT change this host's "2 of 11 observable" verdict. Every daily entry here is `launchd: true`, and that lane reads completion sentinels instead of artifacts, so it never reaches this matcher. The bug is real for any non-launchd daily job, and the launchd routing is a separate question. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(health-check): a launchd job's dated artifact is evidence too The collector already falls back one way — a non-launchd job with no artifact reads its completion sentinel — but not the other. A launchd job with no sentinel never looks at `results/`, so a job publishing a dated artifact every day reports "no dated artifact, cannot tell whether it ran" forever. The comment two lines below the routing already argued for this: "`launchd` conflates 'runs under launchd' with 'publishes no dated results file'". It describes how a job is SCHEDULED, not what evidence it leaves. Live before -> after on this host: before 2 of 11 daily job(s) observable, ALL ON SCHEDULE after loop-engineering-digest: no output today, 976 min past due That job's artifacts run through 2026-08-25 and there is none for today. The probe was reporting "all on schedule" while a daily job had been silently dead for 16 hours, because the only lane that could see it was closed by the flag. `stem_declared` now tracks which lane actually produced the evidence rather than assuming `not launchd`, so the missed-today branch stays correctly gated. Stacked on #3439 (compact-dated artifacts) — that fix is what makes these artifacts visible at all; merge it first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * style(health-check): bring two comment blocks under the 2-line cap CI step 8 (Review checks / REVIEW.md) failed on prose-cap: a 3-line block in health-check.py and a 4-line one in the test. The narrative already lives in the PR body, which is where the cap says to put it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(health-check): pin the empty-both-lanes case yixuan-ag2 flagged Non-launchd with a declared artifact and zero observations: `used_artifact_lane` does flip vs the old `not launchd`, but the interpret layer `continue`s on empty artifacts before the stem_declared gate, so the verdict is unchanged. Executed against origin/main and this branch — both report UNCHECKED, not a miss. Two assertions: the end-to-end fixture, and the interpret layer holding everything but stem_declared fixed so the flag provably cannot matter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The sentinel lane needs a script that stamps one; the artifact lane needs a job that publishes something dated. A job doing neither is UNCHECKED forever, which is 6 of 11 on this host even after #3439 and #3440. But every cron job leaves `results/task-cron-<name>-<epoch>.txt` when its result is written, whatever else it publishes — the one completion record that needs no per-job config and no new writer. mtime is the finish, the same choice _daily_completion_minutes already makes for a sentinel with no body timestamp. Live before -> after on this host: before loop-engineering-digest late; unverifiable (6): agentloop-growth-daily, agentloop-site-metrics, learned-skills-scan, money-autopilot-scan, obsidian-dream, posthog-usage-daily after loop-engineering-digest: no output today, 1022 min past due money-autopilot-scan: no output today, 861 min past due (no unverifiable clause at all) 2 of 11 observable on main, 5 after the two parents, 11 with this. And it makes the live money-autopilot-scan outage visible: that job has not fired since 2026-08-24 and nothing reported it, because UNCHECKED reads as healthy. The glob is anchored on the epoch. `task-cron-{job}-*` also matches `task-cron-{job}-extra-...`, so a neighbouring job's records would have vouched for this one — my own control caught that before the first commit. Stacked on #3440 -> #3439. Merge order is 3439, 3440, then this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…3443) * feat(health-check): task-cron records as the third punctuality lane The sentinel lane needs a script that stamps one; the artifact lane needs a job that publishes something dated. A job doing neither is UNCHECKED forever, which is 6 of 11 on this host even after #3439 and #3440. But every cron job leaves `results/task-cron-<name>-<epoch>.txt` when its result is written, whatever else it publishes — the one completion record that needs no per-job config and no new writer. mtime is the finish, the same choice _daily_completion_minutes already makes for a sentinel with no body timestamp. Live before -> after on this host: before loop-engineering-digest late; unverifiable (6): agentloop-growth-daily, agentloop-site-metrics, learned-skills-scan, money-autopilot-scan, obsidian-dream, posthog-usage-daily after loop-engineering-digest: no output today, 1022 min past due money-autopilot-scan: no output today, 861 min past due (no unverifiable clause at all) 2 of 11 observable on main, 5 after the two parents, 11 with this. And it makes the live money-autopilot-scan outage visible: that job has not fired since 2026-08-24 and nothing reported it, because UNCHECKED reads as healthy. The glob is anchored on the epoch. `task-cron-{job}-*` also matches `task-cron-{job}-extra-...`, so a neighbouring job's records would have vouched for this one — my own control caught that before the first commit. Stacked on #3440 -> #3439. Merge order is 3439, 3440, then this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Bind the task-cron reader to the writer's slugging contract The writer slugifies a configured cron name before building `task-cron-<slug>-<stamp>`; the new punctuality lane globbed the RAW name, so every job whose name needs slugification wrote under one string and was read under another and stayed UNCHECKED forever. Interpolating the raw name into a glob also injected path separators and wildcards: `a/b` searched a subdirectory, `mo*ney` matched arbitrary neighbours. Measured on the pre-fix reader, one record present per job: 'Money scan' -> Money-scan found 0 'a/b' -> a-b found 0 '...' -> unnamed found 0 'money.autopilot.scan' -> money-autopilot-scan found 0 'money--scan' -> money-scan found 0 'money-autopilot-scan' -> money-autopilot-scan found 1 7 of 8 names missed. The duplication was the defect, so rather than copying the regex into the reader this adds `src/cron_task_id.py` as the single owner of the naming contract — slug, task id, discovery glob, and an end-anchored matcher — and points both `cron-runner` and `health-check` at it. Discovery now globs a literal `task-cron-*` and filters with the compiled matcher, so no job name ever reaches a glob. Controls: the reviewer's cases (`Money scan`, `a/b`, all-punctuation) plus `daily.report.job` and `mo*ney` are asserted end to end, configured under the raw name and recorded under the slug. The prefix-neighbour controls are kept and extended — `Money-scan-extra` and `Money-scanner` are both refused. Reverting the reader to the raw-name glob fails 5 of the new assertions, so they catch the regression rather than passing by construction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Regenerate docs/src-map.md for the new naming-contract module `src/cron_task_id.py` is a new module, and the src-map staleness check refuses a diff that adds one without its index entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cron-task-id): require the stamp to end the slug `record_matcher` accepted any non-digit tail after the stamp, so a numeric neighbour still vouched for its prefix: record_matcher("ghost-job").match( "task-cron-ghost-job-2-1787000000000.txt") # True `ghost-job-2` is a separate configured job; `2` was read as the stamp and `-1787000000000.txt` as a suffix. The boundary now accepts `.` or `-` followed by a non-digit, so `-<digits>` marks the digits as the neighbour's slug tail. Kept deliberately wider than "dot or end": measured against 2050 live task-cron-* files, 8 carry a `-late-duplicate.txt` marker directly after the stamp and 1 carries `.no-task.<stamp>.txt`. Requiring a dot would have rejected records this matcher must still claim. Regression controls use the reviewer's exact filename, plus its -late-duplicate variant, plus a control that `ghost-job-2` still matches its own record. Restoring the old pattern fails both neighbour checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cron-task-id): constrain the post-stamp grammar to real suffixes The `-<non-digit>` boundary was still too loose, as reviewed. A neighbour whose slug ends in digits-plus-text passed its digits off as this job's stamp: task_id("ghost-job-2-late", 1787000000000) -> task-cron-ghost-job-2-late-1787000000000 record_matcher("ghost-job").match(... + ".txt") # True Same for `ghost-job-2-text` and `ghost-job-42-backfill`. Consequence is the one this probe exists to prevent: a genuinely missing job reported punctual on an unrelated job's record. `RECORD_SUFFIX` now spells the three suffixes records actually carry, measured over 2056 live and archived records: 2047 `.txt`, 8 `-late-duplicate.txt`, 1 `.no-task.<stamp>.txt`. An unknown suffix fails CLOSED — the record is not counted, so the job reads unverifiable rather than falsely punctual. Regressions are built with `task_id()` rather than hand-typed names, so writer and matcher are exercised against each other, and each neighbour carries a control that it still claims its own record. Restoring the prior pattern fails exactly the three neighbour checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(health-check): the task-cron lane observes cron-runner jobs, not all crons Per @yixuan-ag2's review of #3443: the docstring claimed "every cron job leaves it". Only `cron-runner.py:380` writes that filename shape, so session crons registered via CronCreate never produce one and the lane cannot observe them. Their host measures 41 records, all from launchd:true slugs, while its three UNCHECKED jobs are session crons with 0 records each. Docs-only; the guard already fails closed on an unknown suffix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Why
_daily_artifact_minutesglobs<stem>-20*and then requires the filename to match\d{4}-\d{2}-\d{2}. Both date spellings are in live use inresults/, and the compact one dominates:So every
<stem>-YYYYMMDDartifact was globbed and then silently discarded, anddaily-cron-punctualityreported "no dated artifact, cannot tell whether it ran" for jobs that write one every single day.Before → after, measured against the live results tree
loop-engineering-digest's newest is now('2026-08-25', 364)— minute 364 is 06:04 against a2 6 * * *schedule, i.e. two minutes late. That is the signal the probe exists to produce and could not.Controls, same run: a stem that matches nothing still returns 0, and the hyphenated form still returns 7.
Scoped honestly — this does NOT change this host's headline number
The probe still says 2 of 11 observable after the fix, and I checked that end to end rather than inferring it from the unit-level 0→7. Every daily entry in this host's
crons.jsonislaunchd: true, and that lane reads completion sentinels (state/<job>-YYYY-MM-DD.sentinel) instead of artifacts, so it never reaches this matcher at all.The bug is still real — it misreports any non-launchd daily job whose artifacts use the majority date spelling — but I am not claiming it clears the standing warn. Whether a launchd job that publishes a dated artifact should be allowed to use it as punctuality evidence is a separate design question, and a separate PR;
launchddescribes how a job is scheduled, not what evidence it leaves.Tests
Five cases added to
tests/health-check-daily-punctuality.test.py: compact found and normalised, hyphenated still works, both spellings coexisting in one directory, the returned date parses under the caller'sstrptime(..., "%Y-%m-%d"), and — the control that can fail — widening the date match must not turn the matcher into a bare prefix match on the stem (widget-report-summary.txtandwidget-report-2026notadate.txtare still rejected).Mutation-verified: splicing
origin/main's function body back in takes the suite from 46/46 OK to 2 failures + 1 error; restoring returns 46/46. The siblingdaily-punctuality-completion-sentinelssuite is unaffected and green.🤖 Generated with Claude Code