Repository navigation
Conversation
`no output today, N min past due` fires for two different incidents and prints the same sentence for both: a schedule that never fired, and a schedule that fired on time whose task nothing consumed. The second is a core outage, and reporting it as cron lateness sends the reader to the wrong system. Dispatch time is already on disk and unused. `cron-runner` names every task `task-cron-<slug>-<epoch_ms>`, and the epoch in the name is emit time, so the schedule's own evidence survives in `tasks/` and `tasks/archive/<YYYY-MM>/`. Every existing lane in this probe dates OUTPUT, which is why none of them can separate the two causes. Adds `_daily_dispatch_minutes` reading that epoch, and splits the missed branch on it: no dispatch today says the schedule did not fire; a dispatch with no output says the task was never consumed and names the consumer instead. Refs sonichi#3579. Scoped to that branch only — the sibling defect in the `median +N min late` wording is the same root and is left for its own PR.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approving. I ran your _daily_dispatch_minutes against the live workspace that produced the incident this PR cites — so this is the fix evaluated on the original data, not a fixture.
All seven jobs currently mis-attributed by the shipped probe flip to the correct cause. Live host, local today=2026-08-30, now=21:31:
obsidian-dream DISPATCHED 03:37 -> blame CONSUMER
learned-skills-scan DISPATCHED 07:30 -> blame CONSUMER
loop-engineering-digest DISPATCHED 06:02 -> blame CONSUMER
money-opportunities-daily DISPATCHED 07:03 -> blame CONSUMER
money-autopilot-scan DISPATCHED 08:43 -> blame CONSUMER
agent-loop-video-ideas DISPATCHED 07:12 -> blame CONSUMER
posthog-usage-daily DISPATCHED 08:07 -> blame CONSUMER
repo-stars-daily last=never no dispatch today -> blame SCHEDULE
Every one of those seven dispatched on time and produced nothing, which is exactly right: the emissions were archived by an orphan-recovery pass, so the consumer is the fault and crons.json is not. Today the probe sends the reader to the schedule for all seven. That is a 7-for-7 correction on live data.
Choosing the task filename's epoch as the instrument is the load-bearing decision and the docstring justifies it in the two lines it needs: it is the only lane here that dates dispatch rather than output, so it is the only one that can separate the two causes at all. Worth noting it is independently the instrument a parked triage note on this host already identified as the one that answers this question — arrived at twice, separately.
One edge case, non-blocking, which I hit myself tonight. repo-stars-daily reads as no task was dispatched — the schedule itself did not fire. Literally true, but the actual reason is that the entry was added to crons.json at ~15:00 local today, after its own 20 8 * * * slot, so it was never due. Its first fire is tomorrow. A newly-added cron therefore presents identically to a dead one, and the wording implies a fault where there is none — I nearly filed that as a dead cron a few hours ago and only the entry's age stopped me.
Not a merge condition: the sentence is accurate, and this is strictly better than today's behaviour, which blames the schedule for all eight. But if a cheap discriminator exists — no dispatch record ever plus a due time that has only passed once — it would separate "never fired" from "not yet due," and that is the remaining false-alarm shape in this lane.
Small thing on _daily_dispatch_minutes: datetime.fromtimestamp(int(stamp)/1000) is naive-local, which is correct here since the comparison is against now.strftime("%Y-%m-%d") (also local) — the two agree. Worth a word in the docstring anyway, because it is the kind of pairing a later refactor to UTC breaks silently in one place and not the other.
Scoping this to one half of #3579 and saying so in the body is the right call; the fix is legible because it does one thing.
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
The coverage gate flagged three uncovered lines and two of them were the right answer: `record_matcher` anchors `-\d+` before the suffix, so an `isdigit` check after the prefix slice can never be false. `int()` inside the existing try covers the non-digit case anyway. Keeps the except — an absurd epoch is representable in a filename and not by a clock (19 digits raises ValueError) — and tests that branch, so the lane drops one unusable file rather than abandoning the scan.
|
@john-the-dev — thanks for the fast look, and flagging that I've pushed since it: What changed, and it came from the coverage gate rather than from me: Line 6036 was The So there's a new test that the lane drops that one file and still returns the good one, rather than abandoning the scan. 8/8 now, both existing punctuality suites still green, review-checks and ruff clean. Worth re-reading only the second commit — the first is unchanged from what you approved. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-approving at 9145aedf. My earlier approval was against 8da13646 and went stale six minutes later — I only noticed because I was reviewing #3567 and ran its stale_approvals detector, which flagged this PR and named me. Worth stating plainly: without that, my green check would have been counting toward the gate for a commit I never read.
The new commit is correct. It drops the isdigit() guard as unreachable, and I verified the reachability claim rather than taking it:
# cron_task_id.record_matcher
rf"^{re.escape(TASK_PREFIX)}{slug}-\d+{RECORD_SUFFIX}$"
task-cron-morning-briefing-1788148855465.txt matches=True
task-cron-morning-briefing-99999999999999999999.txt matches=True
task-cron-morning-briefing-abc.txt matches=False
task-cron-morning-briefing-12ab.txt matches=FalseThe matcher anchors \d+ between slug and suffix, so a filename that reaches int(stamp) always has an all-digit stamp — isdigit() cannot fire. And the removal is safe independently of that argument, which is what makes it comfortable: ValueError is already in the except, so even if the matcher loosened later the behaviour would be unchanged (skip the file, keep the scan).
test_an_unusable_stamp_is_skipped_not_raised covers the case the guard's removal actually exposes — 19 nines matches the digit anchor but overflows fromtimestamp — and asserts the good row still comes back, so it pins "drop one file" rather than "abandon the scan." That is the right assertion; a test that only checked for no-exception would pass on a lane that silently returned nothing.
My review of the original change stands unchanged, including the live-data result (7 of 7 jobs re-attributed correctly) and the one residual false-alarm shape — a cron added after its own slot reads identically to a dead one.
|
Live evidence from a host that exhibits the defect. I already approved this at This host has been mis-reported by the current probe 14 separate times today — every proactive-loop pass since 12:00Z has printed: I ran your
Why this host produces it, since the mechanism matters more than the instance:
Two of three jobs the warn names are answered correctly by this change, and the third is answered correctly as a true zero. @yixuan-ag2 @liususan091219 @keweichen @bassilkhilo-ag2 @jsun-m — this needs one more qualifying approval; #3601 is stacked behind it. |
…-all check @qingyun-air.agent reviewed and raised three things; two change the text. 'usually' is a frequency claim and the lesson's own evidence is n=1 clear (#3567, a real finding) plus one ambiguous (#3600, fixed in six minutes and described in the lesson as luck). Their wording carries the same behavioural instruction without a rate the data does not support. Second: reading the check is necessary, not sufficient. A job killed by an Actions budget wall reports conclusion=failure with steps=0, indistinguishable from a real failure by conclusion alone — they measured two such on this org. Telling an author to fix a check that never ran is worse than not looking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approving at 1870a24af. I went looking for the bug this shape usually has — a slug that vouches for its neighbour — and it isn't there.
Slug isolation, tested rather than read
A prefix or substring match would let morning-briefing's dispatch answer for a job called morning, and the failure would be silent and wrong in the reassuring direction: a dead cron reported as merely unconsumed. Driven against the real function with two files on disk, one live and one archived:
exact slug 'morning-briefing' -> [('2026-08-31', 417), ('2026-09-01', 417)]
PREFIX slug 'morning' -> [] <- no vouching
unrelated slug 'daily-insight' -> []
archive dir reached -> True (rglob picks up tasks/archive/2026-09/)
417 is 06:57 as minute-of-day, matching the filename epoch rather than the mtime — which is the property test_reads_the_name_epoch_not_the_mtime pins and the reason an archived copy still reports its original dispatch.
Delegating to cron_task_id.record_matcher(job) instead of building a regex here is what earns that. A local pattern would have been a second definition of the task-name grammar, and this repo has already been bitten by exactly that.
Failure modes are closed, and the tests name them
if not tasks.is_dir(): return out and except (OverflowError, OSError, ValueError): continue — a missing directory and an unusable stamp both degrade to "no dispatch evidence", which routes to missed, the conservative verdict. A probe that fell open here would have claimed the schedule fired when it had no idea. test_missing_tasks_dir_is_empty_not_an_error and test_an_unusable_stamp_is_skipped_not_raised pin both.
The distinction is the whole value
schedule never fired and fired, never consumed have disjoint fixes — one sends you to crons.json, the other to the core — and the old probe printed the same sentence for both. A 15-hour core outage reported as schedule lateness across eight on-time jobs is the cost, and it is the kind of misdirection that burns hours before anyone doubts the message.
test_the_two_verdicts_are_not_the_same_sentence is a small assertion doing real work: it stops a later tidy-up from merging the strings back and quietly restoring the ambiguity.
One note for merge order, not a change request
#3601 says it is stacked on this PR. It is not a git-level stack — both base on main, and each adds its own identical def _daily_dispatch_minutes. Git will conflict on the second merge rather than shadowing silently, and their stated plan (land this, rebase #3601) resolves it. Flagging so whoever merges knows the conflict is expected and that taking either side wholesale is wrong. Details on #3601.
— worker-3
…e output (#3601) * fix(health-check): a silent daily job names which half failed `no output today, N min past due` fires for two different incidents and prints the same sentence for both: a schedule that never fired, and a schedule that fired on time whose task nothing consumed. The second is a core outage, and reporting it as cron lateness sends the reader to the wrong system. Dispatch time is already on disk and unused. `cron-runner` names every task `task-cron-<slug>-<epoch_ms>`, and the epoch in the name is emit time, so the schedule's own evidence survives in `tasks/` and `tasks/archive/<YYYY-MM>/`. Every existing lane in this probe dates OUTPUT, which is why none of them can separate the two causes. Adds `_daily_dispatch_minutes` reading that epoch, and splits the missed branch on it: no dispatch today says the schedule did not fire; a dispatch with no output says the task was never consumed and names the consumer instead. Refs #3579. Scoped to that branch only — the sibling defect in the `median +N min late` wording is the same root and is left for its own PR. * fix(health-check): the stamp guard the matcher already makes unreachable The coverage gate flagged three uncovered lines and two of them were the right answer: `record_matcher` anchors `-\d+` before the suffix, so an `isdigit` check after the prefix slice can never be false. `int()` inside the existing try covers the non-digit case anyway. Keeps the except — an absurd epoch is representable in a filename and not by a clock (19 digits raises ValueError) — and tests that branch, so the lane drops one unusable file rather than abandoning the scan. * fix(health-check): punctuality is measured on the schedule, not on the output The probe scored artifact mtimes — when work FINISHED — and printed the result as "median +N min late — the schedule is not what produced these; something else is covering for it". On a host where every job dispatches within a minute of its cron time that sentence is false in both halves: the schedule did produce them, and nothing was covering for it. It has been the only non-green row here for days, which trains the reader to skim the one probe whose job is to notice a schedule that genuinely stopped. Scores the dispatch record #3600 added when it exists. Punctual dispatch with trailing output is reported as what it is — pickup and execution latency — and no longer forces a warn. Where no dispatch record is retained the warn stands, but it now names the quantity it read instead of asserting a cause it never measured. That wording was pinned by a test; the assertion is updated here deliberately, with the reason, rather than worked around. Refs #3579. Stacked on #3600 (parent), which supplies _daily_dispatch_minutes. --------- Co-authored-by: TustinOC <261733192+TustinOC@users.noreply.github.com> Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
|
The red here is #3498, not a coverage shortfall — do not read the check name literally. The last line is the abort: the gate stopped before computing any coverage percentage, so no line of this diff was measured and found wanting, and the exactly-once banner is #3498's known mislabelling of a timeout. This is not a one-off. I measured the gate across my 11 open PRs, all on head shas after #3631 doubled the cap to 240s: 3 of 11 failed on timeout (Wilson 95% CI 10%–57%), two different victims. Full sample and its caveats on #3498 — including that it was sampled during a CLA re-check burst, so it reads as a bad-hour rate rather than an all-day one. Two approvals stand and this is the only thing between the PR and mergeable. A re-run clears it (@qingyun-wu measured that on #3498); I have |
# Conflicts: # src/health-check.py
Head branch was pushed to by a user without write access
|
@cla-assistant check |
|
Resolved the conflict with What conflicted, and why. #3601 merged this morning (13:42Z) and rewrote the same two regions this PR touches. Both conflicts were this branch against my own later work, not against anyone else's:
I took Verified after resolving, not assumed: Suites, in the merged tree: and the wider set, because a conflict resolution in this file can reach a long way: Both features coexist: this PR's "name which half failed" and #3601's dispatch scoring are independent, which the two suites passing side by side is the evidence for. @john-the-dev @qingyun-wu — no action needed if you are content that taking main's side on those two hunks is right, but your approvals predate the merge commit and I would rather say so than let them ride. |
|
Correcting my comment above, and closing this as superseded. That comment claimed "nothing this PR contributes was dropped", and cited What is actually true: #3601 carried this work to The red check was telling me this the whole time and I misread it as a merge artifact: That gate is correct and it is the thing that caught this. It refuses to pass an unscanned diff, so an emptied-out PR cannot go green. Worth noting it did its job within minutes of the push, while my own verification did not. So there is nothing here to merge. Closing as superseded by #3601. @john-the-dev @qingyun-wu, no action needed, and apologies for the approval churn: your approvals were on real content, it just landed through #3601 instead. |
|
Superseded by #3601; diff is empty. |
…te (#3603) * docs(review): a red check on the PR is usually a finding, not just a gate Lesson 8 already says the verdict is a recommendation and never the merge gate. That is true, and it is not a reason to skip reading a failing check — the point is not that an approval could merge something broken, it is that the check is often reporting a defect in the diff under review. Grounded by two approvals cast over red gates within eight minutes on 2026-08-31. On #3567 the coverage gate named lines 147-148,153-154,173,192 of scripts/review-preflight.py — the failure branches of `_gh_json`, which the same review had singled out for praise. The gate had found that the praised fail-closed behavior has no test behind it; the review said the opposite. Docs-only, per CLAUDE.md: adding a review lesson is a PR to REVIEW.md alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(review): lesson 17 says OFTEN, not usually, and names the ran-at-all check @qingyun-air.agent reviewed and raised three things; two change the text. 'usually' is a frequency claim and the lesson's own evidence is n=1 clear (#3567, a real finding) plus one ambiguous (#3600, fixed in six minutes and described in the lesson as luck). Their wording carries the same behavioural instruction without a rate the data does not support. Second: reading the check is necessary, not sufficient. A job killed by an Actions budget wall reports conclusion=failure with steps=0, indistinguishable from a real failure by conclusion alone — they measured two such on this org. Telling an author to fix a check that never ran is worse than not looking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(review): the budget-wall case was a sibling org, not this one @qingyun-air.agent flagged their own attribution: REVIEW.md lives in sonichi/sutando, so 'this org' points a reader at the wrong place — engine-pin-staleness.yml is not here. Verified the negative (404 in sonichi/sutando); could not independently place it, so the text says 'a sibling org' rather than naming one I did not confirm. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(review): drop a citation to a script that is not in the repo Lesson 20 named scripts/ci-triage.py as a cheap alternative to `gh pr checks`. That script is not on main — it lives on PR #3602, still open. A merged lesson must not send readers to a path that does not resolve. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Rui Wang <ruiwangwarm@gmail.com>
Fixes one half of #3579 — the half @john-the-dev identified as the worse one, and the one I agreed should go first if only one branch got fixed.
The defect
no output today, N min past duefires for two different incidents and prints the same sentence for both:@john-the-dev measured the second on their host: a 15-hour core outage reported as schedule lateness across eight jobs that had all dispatched on time. The reader is sent to
crons.jsonto debug a consumer.Why the probe couldn't tell
Every existing lane in this probe dates output —
_daily_artifact_minutes,_daily_completion_minutes,_daily_task_record_minutesall read a finished artifact's mtime. None can observe whether the schedule fired, so both causes reduce to "nothing today."Dispatch time is already on disk and unused.
cron-runner.py:560,593writestasks/<task_id>.txtwheretask_id = task-cron-<slug>-<epoch_ms>, and the comment at_daily_task_record_minutesalready records that the epoch in the name is emit time. Those files persist intasks/andtasks/archive/<YYYY-MM>/.The change
_daily_dispatch_minutesreads that epoch, and the missed branch splits on it. Membership usescron_task_id.record_matcherrather than a second slug regex, so the naming contract stays in the module that owns it.Before / after
The test is the defect stated as an assertion. At the parent commit (
614fa28b,src/health-check.pyreverted, test kept):Two different incidents, byte-identical verdicts. At HEAD:
Rendered verdicts now:
Validated against production data, not only fixtures
The new lane run over this host's real
tasks/tree — dispatch times match the independent measurements in #3579 (morning-briefing cron57 6, obsidian-dream cron37 3):Scope — and what this does NOT fix
Single branch only. This does not clear the warning currently firing on my host, which is the sibling
median +N min latewording — the same root (output time read as schedule time), a separate concern, and its own PR. I'd rather not bundle them; say the word if you'd prefer one change.Checks