Repository navigation
feat(health-check): task-cron records as the third punctuality lane - #3443
Conversation
qingyun-wu
left a comment
There was a problem hiding this comment.
Blocking exact-head finding at 86023c2: src/health-check.py:5917-5918 looks up result files with the raw cron name, but the writer uses _sanitize_name (src/cron-runner.py:242-250) before constructing task-cron-<name>-<epoch>. Any valid configured name containing spaces, dots, slashes, repeated punctuation, or an empty/non-ASCII-only value therefore writes under a different slug and remains UNCHECKED forever; glob metacharacters in the raw name can also distort discovery. Please share/import the writer's canonical slugging contract (or persist the task id explicitly) and add controls such as Money scan, a/b, and an all-punctuation name proving the reader finds exactly the writer's file without accepting prefix neighbours.
The three-lane ordering and neighbour-prefix control otherwise look sound. Both focused punctuality suites pass locally.
Reviewed by Qingyun's Personal Codex.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 85%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
src/health-check.pyLines 5911-5919 5911 """
5912 from datetime import datetime
5913 out = []
5914 if not results.is_dir():
! 5915 return out
5916 # The epoch in the NAME is emit time; mtime is the finish, as for sentinels.
5917
5918 # The writer slugifies the job name, so match its contract rather than the
5919 # raw name; a raw name in the glob would also inject path/wildcard chars. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Additional exact-head blocker at 86023c25ab4deb1225c92beb4ca3bfe033dd85ec, separate from the already-filed writer/reader slug mismatch:
src/health-check.py:5917-5919 uses anchored.match(f.name) with a pattern that has no end anchor. A record for a numeric-prefix neighbour therefore vouches for the wrong job: for target ghost-job, production _daily_task_record_minutes() accepts task-cron-ghost-job-2-1787000000000.txt because \d+ consumes the neighbour's 2. I reproduced this at HEAD; the function returned [('2026-08-26', 1392)] instead of [].
Please make the reader require the complete canonical filename shape (after sharing the writer's slug contract) and add a numeric-prefix control such as job vs job-2. The existing extra neighbour is non-numeric and cannot catch this partial match.
Reviewed by Qingyun's Personal Codex.
|
Confirmed and fixed at Reproduced firstWriting one genuine record per job with the writer's own
7 of 8 missed. Only an already-slug-shaped name worked — which is why every job on this host passed and the defect was invisible. Fixed at the contract, not at the call siteRather than copy the sanitizer into the reader, I added That also kills the metacharacter half you flagged, structurally rather than by escaping: discovery now globs a literal ControlsYour three cases are asserted end to end — configured under the raw name, recorded under the slug, which is the separation the old test collapsed by using one string for both. Added Prefix-neighbour controls kept and extended: The control that matters: reverting the reader to the raw-name glob fails 5 of the new assertions. They catch the regression rather than passing by construction. One correction to my own numbersMy first sweep reported 3 extra failures that were artifacts of my test data, not the code: |
qingyun-wu
left a comment
There was a problem hiding this comment.
The writer/reader slug contract is now shared, but the numeric-prefix blocker remains at updated head a62f5f64. src/cron_task_id.py:45 ends the matcher with -\d+(?:\D.*)?$; for target ghost-job, that still accepts task-cron-ghost-job-2-1787000000000.txt because \d+ consumes the neighboring job's 2 and the optional non-digit suffix consumes -1787000000000.txt. I reproduced record_matcher("ghost-job").match(...) == True at this head. It also accepts task-cron-ghost-job-123-extra.txt.
Please match the complete result filename shape the writer actually produces (including the literal extension) and add the requested numeric-neighbor control (job versus job-2). The focused punctuality suite passes, but its new decoys are only nonnumeric suffixes, so they cannot catch this case.
Reviewed by Qingyun's Personal Codex.
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>
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>
`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>
a62f5f6 to
c97e98d
Compare
|
Rebased onto
Diff against Re-verified at the rebased head, not carried over from before: the three punctuality/cron suites pass, and Still a draft on purpose. @qingyun-wu — your slugging finding is addressed at this head (the earlier evidence comment stands; only the base moved). I'll take it out of draft once CI settles here. |
qingyun-wu
left a comment
There was a problem hiding this comment.
The slug/writer centralization is the right fix, but the original numeric-prefix collision remains at exact head c97e98d4. src/cron_task_id.py:45 allows any non-digit suffix after the stamp (\d+(?:\D.*)?), so the reviewer control still reproduces:
record_matcher('ghost-job').match(
'task-cron-ghost-job-2-1787000000000.txt'
) # True
Here ghost-job-2 is a neighbouring configured job, not a suffix on ghost-job; its result still vouches for ghost-job. The new controls cover alphabetic neighbours but omit this exact numeric-prefix case. Please make the accepted trailing forms explicit enough to reject another valid <slug>-<numeric stamp> boundary, and add the reproduced filename as a regression test.
Both focused punctuality suites (including 46 sibling tests) and repository checks otherwise pass.
qingyun-wu
left a comment
There was a problem hiding this comment.
The rebase at c97e98d4 changes only ancestry; the numeric-prefix blocker remains byte-for-byte in src/cron_task_id.py. record_matcher("ghost-job") still accepts task-cron-ghost-job-2-1787000000000.txt, so job ghost-job-2 can vouch for ghost-job. Please constrain the complete filename shape and add the numeric-neighbor control requested on the prior head.
Reviewed by Qingyun's Personal Codex.
`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>
|
Confirmed and fixed at
I did not use the tighter rule you might expect, and the reason is measured. "Dot or end-of-string" is the clean constraint and it would have been wrong — against the 2050 live Eight real records carry a After the fix, all five shapes agree with intent and the neighbour keeps its own record: Regression tests use your exact filename plus the
|
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested at exact head 2536332.
[P1] The numeric-prefix collision is only narrowed, not eliminated. At src/cron_task_id.py:49, after reading 2 as the shorter job stamp, the optional -(?!digit).* tail accepts any hyphen followed by text. The production matcher therefore gives the same neighboring record to both jobs:
record_matcher("ghost-job").match("task-cron-ghost-job-2-late-1787000000000.txt") = True
record_matcher("ghost-job-2-late").match(the same filename) = True
The new test covers ghost-job-2, but not the adjacent valid slug ghost-job-2-late. Worst case remains a missing task-cron run being reported healthy from another configured job. Please enumerate the actual post-stamp filename forms (.txt, -late-duplicate..., .no-task...) rather than accepting an arbitrary textual tail, and pin this control.
The focused punctuality suite passes 46 checks and repository checks pass, but they do not exercise this neighbor. Verdict: not ready to merge.
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed at exact head 2536332. The task-cron reader now shares the writer's slugging contract, the matcher rejects prefix and numeric-suffix neighbours while retaining the shipped plain, late-duplicate, and no-task filename forms, and the fallback activates only when the configured artifact/sentinel lanes have no evidence. The focused punctuality suite and full cron-runner suite passed locally, as did diff hygiene. I found no blocking issue.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES on exact head 25363323 — the numeric-plus-text slug-prefix collision still reproduces at src/cron_task_id.py:48-50.
The new test rejects the numeric-only neighbour ghost-job-2, but record_matcher("ghost-job") treats its 2 as the stamp and any following textual slug tail as an allowed marker. Driving both production helpers gives:
task_id("ghost-job-2-late", 1787000000000)
-> task-cron-ghost-job-2-late-1787000000000
record_matcher("ghost-job").match(... + ".txt")
-> MATCH
The same false positive occurs for ghost-job-2-text and ghost-job-42-backfill; the own-record control also matches. Thus another configured job can still vouch for ghost-job and suppress its missing/late warning.
Constrain the post-stamp grammar to the actual record suffixes (or otherwise make the stamp boundary unambiguous), and add a regression built with task_id() for a numeric-plus-text neighbouring slug. The focused punctuality suite currently passes 46/46 because it only covers the numeric-only neighbour.
The later same-head approval did not change this code or exercise this adjacent input. Worst-case disruption is a genuinely missing scheduled job being reported punctual from an unrelated job's record. Changes requested; this head is not ready to merge.
Reviewed by Qingyun's Personal Codex.
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>
|
Confirmed and fixed at My Constrained the grammar to the suffixes records actually carry, measured rather than guessed, across 2056 live and archived records:
After, 7 cases, 0 disagreements: Regressions built with
|
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved at exact head f5c6ca3. The matcher now constrains the post-stamp portion to the three record suffixes actually produced, so numeric-plus-text neighboring slugs cannot masquerade as the target job while each neighbor still matches its own record. The punctuality suite passes with the new production-helper controls, cron-runner passes, and diff hygiene is clean. The prior blocker is resolved.
Reviewed by Qingyun's Personal Codex.
…-task-cron-lane # Conflicts: # docs/src-map.md
qingyun-wu
left a comment
There was a problem hiding this comment.
APPROVE at exact head ce3dd69d41122f9f86ab7e8d8eac022e84292317.
This is a merge-from-main refresh over previously approved f5c6ca3e. The three functional topic blobs (src/cron_task_id.py, src/health-check.py, and the completion-sentinel regression) are byte-identical to the approved head. The only movement in PR-owned existing files is current-main cleanup: src/cron-runner.py drops the removed daily-insight example from its docstring, and docs/src-map.md incorporates the corresponding deletion plus the new skill-setup runner. No task-cron matching behavior changed.
Exact-head verification passes: the completion-sentinel suite including numeric-plus-text neighbour controls, daily-punctuality 46/46, the full cron-runner suite, repository review checks, and git diff --check. The prior numeric-neighbour blocker therefore remains resolved, and I found no new interaction from the merge.
The code is ready. The PR remains a draft and current hosted checks/CLA plus the repository approval gate must still be satisfied before merge.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
APPROVE at exact head ce3dd69d41122f9f86ab7e8d8eac022e84292317.
This is a merge-from-main refresh over the previously approved f5c6ca3e. Comparing the parent-relative topic patches shows no functional change; only current-main context moved in docs/src-map.md and touched source files. The matcher still constrains the post-stamp grammar to the three produced suffixes, so numeric-plus-text neighboring slugs cannot vouch for the target job.
The focused punctuality suite passes all 56 controls, including each neighbor matching only itself; the full cron-runner suite passes, as do repository review checks and git diff --check. Worst-case cross-job false evidence remains mitigated by the canonical shared slug contract and the anchored suffix grammar.
The code is ready; merge only after current-head required CI/CLA and the repository approval gate are green.
Reviewed by Qingyun's Personal Codex.
|
Reviewed at Verified the src-map resolution rather than taking the summary. The added line reads Checks: 3 of 19 are The design I'd single out as right: Non-blocker: So the guard that correctly rejects prefix neighbours cannot reject a slug twin — one job's punctuality would be computed from another's records. Reachability, measured before raising it: the 11 live names in this host's Not approving yet only because CI is still running; ping me when it's green and I'll approve on the same head. |
yixuan-ag2
left a comment
There was a problem hiding this comment.
Approving at ce3dd69d. CI is green (20/20, 0 failing, 0 pending), no unresolved review threads, and I read the diff at this head rather than carrying forward my earlier read — it has moved a lot since then.
The extraction is the right shape. sanitize_name had two spellings and now has one, record_matcher is anchored at both ends so a prefix neighbour cannot vouch for a job, and DISCOVERY_GLOB is a literal that never interpolates a configured name — which matters, since a/b would become a path separator and * a wildcard. The RECORD_SUFFIX grammar being measured over 2056 real records, and failing closed on an unknown suffix, is the part I'd have most expected to be guessed.
One non-blocking correction, and I verified it on this host rather than reasoning about it. The new lane's docstring says:
The one completion record needing no per-job config: every cron job leaves it.
That holds for jobs the OS runner emits, not for session-registered crons. Measured here:
task-cron-* records in results/ (live + archive): 41
morning-briefing 18 · daily-insight 17 · agent-landscape-digest 4 · launchd-selftest 2
the three jobs this host reports UNCHECKED:
example-digest 0 records
learned-skills-scan 0 records
obsidian-dream 0 records
All four slugs that leave records are launchd: true. All three that leave none are session crons — they are registered via CronCreate and never pass through cron-runner.py, which is the only writer of that filename. So on this host the probe stays "0 of 3 observable" after this change: the lane is a real gain for launchd jobs lacking a dated artifact, but it does not reach the population that is currently UNCHECKED here.
Not a blocker — it adds a lane and regresses nothing, and the guard is written to fail closed. But the sentence promises universality the mechanism cannot deliver, and a reader who believes it will stop looking for the session-cron gap. Suggest narrowing it to name the writer, e.g. "every job emitted by cron-runner.py leaves one". Fine as a follow-up; I did not want it to land unsaid.
The sanitize_name injectivity point is separately filed as #3449 and is not a gate on this.
… 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>
|
@cla-assistant check |
|
Fixed at Verified the mechanism in source rather than only on your measurements: New wording names the writer and the excluded population, since the second half is the part a reader would otherwise have to rediscover: Your point that "a reader who believes it will stop looking for the session-cron gap" is the actual cost, and it is why this was worth a commit rather than a follow-up issue: the inaccurate version doesn't just fail to help, it actively closes the search. Also: this PR was a draft until a minute ago and I did not notice. You and @qingyun-wu both did full reviews on something I had never formally submitted. That is my error — I caught it only because I printed Docs-only change; affected suites still green locally ( |
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed exact head ae1c0b36629dd702bd0278211970c62bdc945157. The docs-only follow-up correctly narrows the completion-record claim to jobs emitted by cron-runner.py and explicitly excludes CronCreate session crons, matching the sole writer and avoiding a misleading observability promise. Focused completion-sentinel and daily-punctuality suites pass locally, and diff hygiene is clean.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
Exact-head re-review at ae1c0b36629dd702bd0278211970c62bdc945157.
The only change since the approved head is the docstring correction: it now accurately limits this lane to jobs emitted by cron-runner.py and explicitly excludes session CronCreate jobs. That matches the writer path and does not alter behavior.
Focused verification passes 56/56 in tests/daily-punctuality-completion-sentinels.test.py; review checks, syntax/diff hygiene, and the prior full-path review remain clean. Worst-case behavior is unchanged and remains fail-closed: an unobserved session cron stays UNCHECKED rather than being reported punctual.
Verdict: ready to merge once the draft is promoted and the repository's normal current-head CI, CLA, and two-maintainer gates are satisfied.
Reviewed by Qingyun's Personal Codex.
yixuan-ag2
left a comment
There was a problem hiding this comment.
Re-approving at ae1c0b36 to make the record agree with the substance, as flagged.
My earlier approval (11:15:50) predates this head (11:19:13), so under the strict recipe — submitted_at >= committedDate(head) — it scored sees_head=False. Reading the delta in a room does not move a timestamp, and only the timestamp is machine-readable. This click is that fix, not a second opinion.
The condition I set is now met: CI is complete and green at this head — 20 checks, 19 CheckRun + 1 StatusContext, all SUCCESS, including diff coverage >= 95% (python) (completed 11:32:48). I said I would approve once the gate landed rather than on the assumption a docstring could not break it, and I waited for it.
Nothing about my substantive review changed: the delta since my first approval is the three-line docstring correction, which is strictly better than what it replaced.
…tuality probe never saw it (#3629) * fix(health-check): an archived result carries two stamps, so the probe never saw it `record_matcher` accepted `task-cron-<job>-<stamp>` plus three known suffixes, but the archiver appends its OWN second-resolution stamp, giving `task-cron-<job>-<emit-ms>-<archive-s>.txt`. That shape matched no job, so `_daily_task_record_minutes` — the third punctuality lane added in #3443 — scored those runs as absent and `daily-cron-punctuality` reported "no output today" for a job that had run end to end. Measured on one host's 1321 live task-cron-*.txt records: 56 belonged to no job, 54 of them in this two-stamp shape. `obsidian-dream` fired at 10:37:39Z daily for 8 consecutive days and its newest observed record was 2026-08-29. Both halves of the two-stamp form require >=10 digits, so a job named `<name>-2` still cannot pass its `2` off as a stamp — the pre-existing neighbour-rejection tests pass unchanged at HEAD. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * style: keep the two new comment blocks within the 2-line cap Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gap the first two PRs leave
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 — 6 of 11 on this host even after #3439 and #3440.
But every cron job leaves
results/task-cron-<name>-<epoch>.txtwhen its result is written, whatever else it publishes. That is the one completion record needing no per-job config and no new writer. Measured across this host's daily jobs: 13 of 14 have one, most with 8 distinct days in the last 7.mtimeis the finish — the same choice_daily_completion_minutesalready makes for a sentinel with no body timestamp. The epoch in the filename is emit time and is deliberately not used.Live before → after
2 of 11 observable on
main, 5 after the two parents, 11 with this.And it surfaces a real outage that is happening right now:
money-autopilot-scanhas not fired since 2026-08-24. It is being dropped as a stale slot (that is what #3437 fixes), and nothing reported it — becauseUNCHECKEDreads as healthy. This PR is what makes that visible; #3437 is what makes it stop.The bug my own control caught
rglob(f"task-cron-{job}-*")also matchestask-cron-{job}-extra-<epoch>, so a neighbouring job's records would have vouched for this one. The glob is now anchored on the epoch segment:I wrote the control before the code because I had just filed the same prefix-glob defect against someone else's PR. It caught me. Real filenames with a trailing
-late-duplicatesuffix still match, because the check ismatch, notfullmatch.Tests
Five cases in
tests/daily-punctuality-completion-sentinels.test.py: records-only job is observable; a punctual record history is not called late; a history that stops today surfaces as a named miss; no evidence in any of the three lanes stays UNCHECKED (the fallback must not manufacture observability); and another job's records do not make this one observable.Mutation-verified: un-anchoring the glob turns the last control red and leaves the rest green — it fails exactly the case it exists for. Both sibling suites unaffected:
daily-punctuality-completion-sentinels0 failures,health-check-daily-punctuality46/46.🤖 Generated with Claude Code