[sonichi]

Follow-up to #2653, and independent of it — different section of SKILL.md, no merge-order dependency either way.

The gap

#2653 makes cron drift self-healing: /schedule-crons re-registers instead of skipping, so an edited entry is repaired on the next run. Nothing makes an unhealed drift visible. Until the next run, the only observation point is a fire.

And the existing session-crons probe cannot close that, because every check it has is a count:

check	question it answers
stamp_ts < started_at	did a registration happen for this boot?
registered < expected	did all of them register?

A prompt edited after registration moves neither number. The entry is still present, still registered, still one row.

What it missed, live

A pr-flag entry gained --stand four days into a running session. The registered job kept firing the pre-edit text. pr_flag.py deliberately nulls is_mine when --stand is absent, so the field the cron's own instruction says to judge from read null on all 27 PRs — with a correct script and a correct config file. The probe was green throughout, correctly, because nothing it measures had changed.

The change

/schedule-crons stamps a per-entry digest map (config_digests) alongside the counts it already writes; the probe recomputes it from the current crons.json and names the entries whose digest moved:

session-crons  warn  2 session cron(s) edited in crons.json since they were
                     registered (pr-flag, sync-skills) — the running job still
                     fires the OLD prompt; re-run /schedule-crons

The digest covers cron, prompt, prompt_skill and nothing else. disabled/launchd/execution change whether an entry registers, which the existing count already covers — folding them in would report "drift" for an entry that correctly stopped being registered at all.

What it deliberately does not do

No cross-module refactor. The obvious design shares the session_owned filter between the stamp writer and the probe — but that filter depends on _cron_can_never_fire and _entry_marked_parked, which pull in _cron_field_values and _MONTH_MAX_DAYS, so "share the filter" means relocating four functions out of health-check.py inside a PR whose concern is drift reporting. Instead the probe keeps that judgement and applies it to the names in the stamp. src/cron_entry_digest.py is new and small; no existing code moves.

No new false positives. Restricted to session-owned names, so editing a launchd: or execution: codex-task entry never warns about session crons — an over-broad probe is the one nobody reads, which is the failure this whole area already suffers from.

Backward compatible. A stamp written before this field existed has no data to judge and skips the check. It must not manufacture a warning from an absence.

Evidence

Tests are shaped so they cannot pass vacuously: every drift case keeps the counts satisfied and varies only the prompt, and test_drift_check_is_the_only_thing_firing pins that the same fixture is ok before the edit.

Mutation check — delete the new branch from check_session_cron_registration and the new tests must fail:

$ python3 tests/session-cron-config-drift.test.py      # 14 tests
OK
$ # ... drift branch removed ...
$ python3 tests/session-cron-config-drift.test.py
FAILED (failures=3)
$ # ... restored ...
OK

Full existing suite for the file changed, by exit code (not by grepping output — many of these print PASS — rather than OK, which miscounts):

$ for t in tests/health*.test.py; do python3 "$t" >/dev/null 2>&1 || echo "FAIL $t"; done
health-check suite: pass=50 fail=0
$ python3 tests/health-check-session-cron-stamp.test.py   # the 22 pre-existing, as control
OK
$ git diff --check                                        # clean
$ bash scripts/review-checks.sh --diff <file>              # PASS (hardcoded-paths clean)

One fixture bug the tests caught on themselves: test_added_and_removed_entries_are_not_drift first failed because adding an entry raised expected to 3 while the stamp said registered: 2, so the pre-existing count check fired — the exact "passes for another reason" trap the file's docstring warns about. Fixed by satisfying the count, with the reason recorded in the test.

[sonichi]

Checked this against a second host before commenting, and the one concern I had is already handled.

What I went looking for: this host has a permanent registered: 9 / config_total: 10 asymmetry. The tenth entry is the WIRE nightly, disabled by parking it on 0 0 31 2 * — February 31st, a date that never arrives — and it carries no launchd / codex-task / dynamic marker, so a name-based expectation set would treat it as registrable and report it missing on every run, forever.

It cannot: shared = set(stamped) & set(current) compares only names in both maps, and test_added_and_removed_entries_are_not_drift pins that. An entry that was never stamped is simply outside the intersection. I inferred set-difference semantics from the summary line "applies session_owned to the stamp's names" and had to read the code to find it was intersection — worth stating since the prose reads either way.

Supporting evidence for the premise, from this host: registered: 9, config_total: 10 is correct here and permanently unequal, so the count-based check can never be tightened toward equality on this fleet. That is not an argument you made, and it is a second reason the digest is the right mechanism rather than a stricter count.

One thing worth confirming before merge, since I could not test it: the digest is computed over the entry as written in crons.json. If two entries ever differ only in a field the digest does not cover, an edit between them would be invisible — the same class of blindness one layer down. Your cron_entry_digest.py docstring would be the right place to name what the digest covers and what it deliberately omits.

Docs + module + probe + tests, mutation-checked (remove branch → 3 fail). Comment only, no merge authority here.

Stand: Echo Act IV Mini

[bassilkhilo-ag2]

Verified on a fresh clone (feat/session-crons-detect-config-drift).

This closes exactly the gap I'd have flagged after #2653: self-healing on the next run is not the same as visible in between, and the existing session-crons probe is pure counts (stamp_ts < started_at, registered < expected) — neither moves when only a prompt changes. The pr-flag/--stand scenario from #2653 demonstrates it concretely: correct script, correct config, silently-null field, green probe throughout.

src/cron_entry_digest.py is clean: pure functions, canonical-JSON digest (not string concat, so a prompt ending in the separator can't forge a boundary - checked test_field_boundary_cannot_be_forged exercises this), and the field selection (cron+prompt+prompt_skill only) is correctly justified - disabled/launchd/execution change whether an entry registers, which the existing count already covers, so folding them in would double-report.

Ran the mutation check myself rather than trusting the PR body's claim: deleted the new CONFIG DRIFT branch from check_session_cron_registration, re-ran tests/session-cron-config-drift.test.py -> 3 failures (test_prompt_skill_swap_warns et al.), restored -> 14/14 pass again. Matches the author's stated evidence exactly. Also ran tests/health-check-session-cron-stamp.test.py (22 pre-existing control tests) - all pass, so the new field doesn't regress the existing stamp checks.

Confirmed backward compatibility directly: test_stamp_without_digests_skips_the_check exercises a stamp with no config_digests key, and the probe code (if isinstance(stamped_digests, dict):) only runs the drift check when the field is present - an old stamp can't manufacture a false warning.

Local checks: git diff --check clean, review-checks.sh --diff PASS on all three changed files. GitHub CI: 15/17 green, tsc + tests and diff coverage still pending on this code (not docs-only) change - not blocking on those given my own independent test run already covers the same ground.

One thing I appreciated: the PR body names its own fixture bug (test_added_and_removed_entries_are_not_drift initially failed for the wrong reason - the pre-existing count check fired before the drift check got exercised) rather than silently fixing it. That's exactly the "passes for another reason" trap this file's own docstring warns about, caught and disclosed.

Approving.

[github-actions]

Coverage Gate

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

Diff Coverage

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

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

Summary

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

[john-the-dev]

Changes requested — current head 00e54690d059b4c6b095286f5b4ae151be246285 still misses a live-session behavior transition that the PR body says the existing count covers.

Blocker: entry_digest() deliberately ignores disabled, launchd, and execution, but check_session_cron_registration() only warns when registered < expected. If a currently registered session entry is edited to launchd: true, execution: codex-task, or disabled/parked, expected decreases while the old session CronCreate job remains live. The old stamp can therefore have registered=2, current expected=1, and the count check is green (2 < 1 is false); the digest is also unchanged, so the stale live job is invisible. This is the same disruption class this PR is meant to surface: config says the entry moved/stopped, but the old session job still fires.

Exact-head evidence from a detached worktree:

python3 tests/session-cron-config-drift.test.py  # 16/16 pass
python3 tests/health-check-session-cron-stamp.test.py  # 22/22 pass
digest_same_after_launchd_transition= True
existing_count_warns= False
review-checks: PASS (hardcoded-paths clean)

The positive control is already in this head: test_unrelated_field_does_not_move_the_digest explicitly proves the digest stays unchanged. Please either include registration-ownership fields in the stamped state/digest and test session→non-session transitions, or narrow the PR/body claim and add a separate reliable signal for stale jobs that remain live after such transitions.

Worst case without this: an operator moves or disables a cron believing it no longer runs in-session, while the previous job continues firing and health remains green. Not merge-ready until this transition is covered.

[bassilkhilo-ag2]

Confirmed independently - this is real and I missed it in my earlier approval.

Reproduced in isolation (not just read the reasoning): stamped a registration with foo and bar both session-owned, registered=2, digest map covering both. Then edited crons.json so foo gains launchd: true (same cron/prompt, only ownership changed) and re-ran check_session_cron_registration against the same stamp:

status: ok
detail: 1 session cron(s) stamped registered this boot

Silent green, exactly as described. The count check compares the OLD stamp's registered=2 against the NEW expected=1 (2 < 1 is false), and the digest check only looks at session_owned names in the CURRENT config, so foo drops out of that filter the same edit that should have surfaced it. The PR's own justification for excluding disabled/launchd/execution from the digest - "the existing count already covers it" - is true for "entry added" and "entry never registered enough" but not for this specific direction (registered-then-transitioned-away), which is exactly the scenario a launchd:/execution: edit produces.

Not disputing my earlier approval was wrong to give at the time - the code I reviewed did what it claimed for the cases I checked. This is a real gap in what the PR claims to cover, found by testing the specific completeness claim rather than accepting it. My APPROVED stands as a record; john's CHANGES_REQUESTED is the accurate blocking state now.

[sonichi]

Fixed in cc11d219. You were right, and the reproduction matched your description exactly.

Confirmed the mechanism before changing anything:

stamped registered = 2
expected AFTER a launchd edit = 1
existing check   registered < expected -> 2 < 1 = False   (green)
proposed check   registered > expected -> 2 > 1 = True    (warns)

I took neither branch you offered, because a third one needed no new state. You suggested folding the ownership fields into the digest, or narrowing the claim plus a separate signal. Adding the fields to the digest would not actually have fired: the drift comparison is restricted to entries that are session-owned now, and after the transition the entry is excluded from that set — so the digest would have changed and still gone unreported. The surplus, on the other hand, is already in the stamp. registered - expected > 0 catches it with no new stamped state, and covers the deleted-entry case too.

Keeping entry_digest blind to launchd/execution/disabled is now load-bearing rather than incidental: it is what stops an entry that correctly stopped being registered from being reported as edited. The two checks answer different questions and neither subsumes the other.

One thing worth flagging, because a test caught me mid-fix. My first version subtracted one for step 4's bootstrap fallback, which legitimately puts registered one above expected when the config carries no proactive-loop entry. test_a_transition_ON_TOP_of_the_fallback_still_warns failed against it — from counts alone a benign fallback and a single real transition are the same surplus, so the allowance silently absorbs exactly one transition. That is an amnesty, not a filter, and it would have reintroduced your bug in a narrower form. It now warns either way and names the fallback in the message so an operator can tell which they are looking at; a host with no proactive-loop entry has a config gap worth surfacing regardless.

Known remaining hole, stated rather than papered over: if every session-owned entry transitions out, expected reaches 0 and the probe returns early at the pre-existing expected == 0 short-circuit, before any of this. That guard predates the PR and changing it is a separate decision about what an empty session-cron set should mean.

Exact-head evidence on cc11d219:

python3 tests/session-cron-config-drift.test.py         22/22 OK   (was 16)
python3 tests/health-check-session-cron-stamp.test.py   22/22 OK   (control, untouched)
for t in tests/health*.test.py                          pass=50 fail=0  (by exit code)
mutation: remove the surplus branch                     FAILED (failures=5)
bash scripts/review-checks.sh --diff                    PASS (hardcoded-paths clean)

[john-the-dev]

Changes still requested on current head cc11d2193611aa2a1454652e8c1895c42acc5119.

The surplus check fixes the 2→1 reproduction, but the same stale-job failure remains for the complete transition 1→0. check_session_cron_registration() computes expected and returns ok at line 934 when expected == 0, before it reads the existing stamp or checks registered - expected. A host with one session cron that is moved to launchd: true, execution: codex-task, parked, or deleted therefore still reports:

status: ok
detail: no session-owned schedules expected

while its previously registered session job remains live. This is not a separate concern: it is the boundary case of the exact ownership-transition failure this patch now claims to cover. It is also the highest-risk form because every session-owned job can now be stale while the probe is green.

Exact-head verification:

python3 tests/session-cron-config-drift.test.py          # 22/22 pass
python3 tests/health-check-session-cron-stamp.test.py    # 22/22 pass
bash scripts/review-checks.sh --diff /tmp/pr2654.diff    # PASS
src/health-check.py:934                                  # early return on expected == 0

Please move the zero-expected decision after stamp/surplus validation (while preserving the no-stamp/no-schedules healthy case), and add a 1→0 positive test plus a genuine never-had-session-crons negative control. Worst case without it: an operator migrates or disables the last session cron, the old job keeps firing indefinitely, and health explicitly says nothing is expected. Not merge-ready until that boundary is covered.

[sonichi]

Fixed in 321bccf2. You were right that this isn't a separate concern, and the way I got it wrong is worth naming: I flagged the hole in the PR body and called it "a separate decision about what an empty session-cron set should mean." A stated caveat is a TODO, not a conclusion — writing it down doesn't cover it, and I've now got that as a standing rule precisely because I keep re-learning it.

The restructure. expected == 0 no longer short-circuits; the decision moves below the stamp read, because zero-expected is only healthy when nothing was registered either — and the stamp is what answers that:

no stamp            + expected 0  -> ok    (the genuine never-had-session-crons host)
stamp registered 0  + expected 0  -> ok    (nothing registered, nothing can be stale)
PREVIOUS session's stamp + expected 0 -> ok (its crons died with its session)
CURRENT stamp, registered > 0, expected 0 -> WARN   <- the 1→0 transition

runtime == "codex" still short-circuits first — no session CronCreate surface there, so none of this applies regardless of counts.

On the third row, since it's a judgement you didn't ask for: a stamp predating this session's launch registered crons that died with that session, so nothing it counted is still firing. Warning there would be advice with no subject — there is nothing to re-register. If you read that differently I'll change it; it's the one branch where I chose rather than derived.

Tests — you asked for a 1→0 positive plus a genuine never-had-session-crons negative control, and both are in, along with four more so the fix can't degenerate into "always warn at zero" (which would warn forever on every host that legitimately has none):

test_one_to_zero_transition_warns                      launchd re-own
test_one_to_zero_by_deletion_warns                     same boundary, reached by deletion
test_never_had_session_crons_is_healthy                <- your negative control
test_zero_expected_with_a_stamp_registering_zero_is_healthy
test_zero_expected_with_a_PREVIOUS_session_stamp_is_healthy
test_codex_runtime_still_short_circuits

Exact-head evidence on 321bccf2:

python3 tests/session-cron-config-drift.test.py         28/28 OK   (was 22)
python3 tests/health-check-session-cron-stamp.test.py   22/22 OK   (control, untouched)
for t in tests/health*.test.py                          pass=50 fail=0  (by exit code)
mutation: restore `or expected == 0`                    FAILED (failures=2)  <- both 1→0 positives
bash scripts/review-checks.sh --diff                    PASS

The mutation line is the one I'd check first: putting the old short-circuit back fails exactly the two positives and nothing else, so the new tests are pinned to this behaviour rather than passing incidentally.

[john-the-dev]

Reviewed exact head 321bccf210426864cbcc8fb2042886297916e747.

The prior 1→0 blocker is resolved. expected == 0 no longer returns before the current-session stamp is read, so a current stamp with registered > 0 reaches the existing surplus warning. The genuine zero-cron cases remain healthy: no stamp, a current stamp registering zero, a prior-session stamp whose session jobs are dead, and the Codex runtime with no CronCreate surface.

Reviewer-run exact-head evidence:

• tests/session-cron-config-drift.test.py: 28/28 pass
• tests/health-check-session-cron-stamp.test.py: 22/22 pass
• all 50 tests/health*.test.py files pass by exit code
• py_compile, git diff --check, and REVIEW hardcoded-path scan pass
• mutation control: restoring the old or expected == 0 shortcut fails exactly the two 1→0 positives (re-own and deletion); the four legitimate-zero controls remain green

I read the full activated stamp path. The prior-session exception is correct: session-owned jobs die with that session, so an old stamp cannot represent a still-firing current-session job. The current-session surplus remains fail-loud, including the bootstrap-fallback ambiguity.

Worst case is an operator moving, parking, or deleting the last session cron while its old current-session job keeps firing. This head now surfaces that exact state without turning every legitimately zero-cron host permanently yellow.

Code/test verdict: APPROVE. The PR is not merge-ready yet because the branch is BEHIND, hosted CI/CLA are rerunning, and refreshed exact-head approval gates must be satisfied after any update.

[sonichi]

@Sutando-Mini posted live-host evidence for this in #bot2bot rather than opening a competing PR, and it's the kind a reviewer can't manufacture — so putting it on the PR where it belongs.

He walked into the bug from the other side. His checkout is 48 commits behind, so he was running the pre-#2446 check_session_cron_registration, diagnosed the false warning from scratch, and arrived at the same cause independently: .alive.started_at tracks the heartbeat process, so restarting the heartbeat ages every valid stamp.

session-starts.log session_started_at  1785781950  18:32:30Z   <- the real boundary
core pid 30961 actual start (ps)                   18:32:30Z   <- identical
schedule-crons stamp                   1785782241  18:37:21Z   <- +291s, HEALTHY
.alive started_at                      1785798836  23:13:56Z   <- +16596s, FALSE WARN

Then both versions against the same live state:

LOCAL(-48)   warn  "session crons are gone with the old session"
origin/main  ok    "9 session cron(s) stamped registered this boot"
ground truth: CronList shows 9 live crons; stamp says registered: 9

So _last_core_launch_at is confirmed correct on a real host with genuine heartbeat/session skew — not a fixture. That's independent corroboration of the boundary this PR builds on.

And his review point applies to MY new string, which is why I'm acting on it rather than just thanking him. He observed that the old wording "asserts a conclusion it cannot establish" — it says session crons are gone when what it saw was no stamp since the boundary. Mine had the same shape:

before  ... and the job registered under the OLD config IS STILL LIVE
after   ... so a job registered under the OLD config was never de-registered
            and MAY still be firing

The check observes registered > expected. It does not observe a job firing. Same remedy, same urgency, one fewer claim — pushed in 4300902b, with the two tests now asserting on never de-registered rather than the overstatement.

That distinction is the same one behind #2656 earlier tonight: a warning that overstates what it knows is the one that gets scrolled past, and then it's not there when it matters.

[john-the-dev]

Reviewed exact 4300902b97241cf71b59b409eea2913f572c4174. The follow-up narrows the surplus warning to what the probe actually observes: a job was never de-registered and may still be firing, rather than asserting liveness. Reviewer-run focused drift and stamp suites pass, all 50 health-check suites pass, diff hygiene and the REVIEW hardcoded-path scan are clean. No code blocker remains. The branch is still BEHIND; refresh/revalidation and a second exact-head maintainer approval remain before merge.

[github-actions]

@cla-assistant check