Repository navigation
fix: detect missing core loop and require host completion proof - #5573
Merged
Merged
Conversation
john-the-dev
requested review from
liususan091219,
qingyun-wu and
sonichi
as code owners
October 15, 2026 17:10
Contributor
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 91%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
skills/schedule-crons/scripts/codex-scheduler.pyLines 466-480 466 if codex_runtime_selected():
467 try:
468 entries = json.loads((workspace / "hosts" / host_label / "crons.json").read_text())
469 if not isinstance(entries, list):
! 470 raise ValueError("crons.json must contain a JSON array")
471 problem = main_loop_problem(entries, "codex")
472 if problem:
473 problems.append(problem)
474 load_jobs(workspace / "hosts" / host_label / "crons.json", include_main_loop=True)
! 475 except (OSError, ValueError, TypeError) as exc:
! 476 problems.append(f"cannot load schedules: {exc}")
477 if state.get("host_label") != host_label:
478 problems.append("scheduler state does not belong to this host")
479 last_tick = parse_iso(state.get("last_tick_at"))
480 if last_tick is not None and last_tick > now: |
qingyun-wu
approved these changes
Oct 15, 2026
qingyun-wu
left a comment
Collaborator
There was a problem hiding this comment.
Approving at 948120c. I found no blockers.
Tests at head, exit codes captured directly:
tests/cron-loop-policy.test.py: Ran 4, OKtests/loop-completion.test.py: Ran 4, OKtests/codex-scheduler.test.py: "All codex-scheduler tests passed."tests/health-check-cron-runner.test.py: Ran 10, OKtests/health-check-cron-schedule-liveness.test.py: Ran 15, OKscripts/review-checks.sh --diff <PR diff>: PASS (hardcoded-paths, root-artifacts and prose-cap clean)
Without the fix: running these tests against origin/main's source, the codex-scheduler and liveness suites fail on import. The cron-runner suite fails on behaviour: test_codex_hourly_success_does_not_hide_a_missing_main_loop and the missing/invalid/owner-exempt config test both get 'ok' != 'down'.
Focus areas
- Shared policy and workers. The loop identity and parking rules live only in
src/cron_loop_policy.py.health-check.pyimports from it and the old copies are deleted.codex-scheduler.pyusesis_core_main_loopandmain_loop_problem. Worker-owned entries are filtered out byentry_owner(e) == CORE, and a test covers a loop withowner="worker-a". - Completion proof.
write_completiontakes a file lock and writes through temp file, fsync andos.replace. The path is per host, a storedhostthat doesn't match is rejected, the host label is validated, and the timestamp must be finite and non-negative. The scheduler caps the timestamp atmin(now, result mtime). - False greens, probed directly against the module.
- A peer host's record under this host's path reads as
None. NaNandversion: trueread asNone.- An invalid record warns, and so does a future-dated one.
- A plain
idleno longer writes proof; only--loop-completefrom idle does. - The legacy
last-loop-okmarker is ignored.
- A peer host's record under this host's path reads as
- Removed health-check code. The cron-parsing helpers moved over unchanged. Two lenient ok paths were dropped on purpose and are documented in the rationale: the fresh-workspace grace and "this build can't write the marker". Both now warn, which is stricter.
Non-blocking notes
src/loop_completion.py:49: thets >= previousguard means a single future-dated write, from a fast clock, blocks every real write after it until the clock catches up. Meanwhile the check can report ok for up to the window past that stale time with no pass running. Reproduced: a write at now+1h, then a write at now; the stored value stays 1h ahead. Consider rejecting apreviousthat lies in the future rather than keeping it.- The "awaiting first proof" grace is measured from
core-status.sh's mtime, so every engine update or checkout reopens up to 7h of ok. It's bounded and documented; just flagging it. check_cron_runner.eligible()(health-check.py ~1108) still has its own loop heuristic, includingprompt == "/proactive-loop", which_core_loop_candidatesdoesn't recognise. The result fails loud, not as a false green, but it is a second definition of "the loop". Worth routing through the shared module in a follow-up.runtime = runtime or resolve_core_runtime(...)now appears twice incheck_cron_runner.--loop-complete: underset -e, a failinghost-labelexits before the idle status is written.- The scheduler treats any result file for the main-loop task as completion, including an early-bail
[no-send]. - Pre-existing, not introduced here:
state/schedules/codex-scheduler.jsonis not per host. Ifstate/is synced, two hosts overwrite each other's job state.
Not verified: a live Codex or Claude core run, Windows paths, the catalog and src-map regeneration, and whether workspace sync carries state/loop-completions/.
— sutando-qingyun-air
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A Codex host with only an hourly job reports healthy even when its proactive loop is missing. Any ordinary task ending in
idlealso refreshes the old global loop marker. This change checks the expected core loop configuration and records actual host-specific pass completion instead.Closes #5569.
Change and review entry points
src/cron_loop_policy.py: one shared core-loop identity/presence and deliberate-parking policy, consumed byhealth-check.pyand scheduler health. The existing conservative parking predicates move here unchanged. Worker-owned loops cannot satisfy or implicitly become the core loop.src/loop_completion.py: the shared schema, numeric/host validation, lock, atomic publication and monotonic writer forstate/loop-completions/<host>.json. Oversized numeric JSON is rejected without crashing health.scripts/core-status.sh: ordinary idle only writes work status; explicitidle "" --loop-completerecords a finished pass. The proactive skill's final command uses it.check_cron_schedule: local proof only; stale, invalid, future-dated, peer and legacy global markers cannot establish liveness. First-proof installation grace remains bounded. Autonomy pause does not disable the health pass.Related #5158 and #5169 handle empty/missing configuration and launch-time seeding. This diagnoses an existing hourly-only configuration without creating jobs or changing install's empty-job policy. Not stacked on either PR.
Before / after evidence
Parent:
e25da60f8; HEAD:948120c694228a0fb6dc361aa60cfda5ac410a2d. The two regressions were run before implementation and rerun at HEAD.Real affected-host scheduler round trip
Used the existing launchd label, with its original plist backed up; no second recurring driver and no live-core checkout swap. The scheduler entrypoint was restarted at each tested candidate.
At
06e43d462, the OS job enqueued the real 17:00 pass at 17:00:05Z. Its real result was published at 17:04:34.655Z; the OS tick at 17:05:01Z recorded that publication timestamp. The original job was restored.After the numeric-validation follow-up, restarted the existing job at exact HEAD
948120c69at 17:08:09Z. It consumed the real result of the already queued 17:05 pass (enqueued by the previous candidate; enqueue code is unchanged). Current-head completion writer and health reader output:Restored the original plist byte-for-byte and verified the normal launchd argv at 17:09:40Z. The live core and bridges were not restarted. This is a real scheduled task/result completion witness, not an owner-message delivery claim; the schedule intentionally uses
[no-send]. Claude's explicit completion wrapper is covered by isolated production-wrapper tests, not a live Claude session.Validation
python3 tests/cron-loop-policy.test.py: 4;loop-completion.test.py: 4;health-check-cron-runner.test.py: 10;health-check-cron-schedule-liveness.test.py: 15 — pass.python3 tests/codex-scheduler.test.py: 19 functions — pass, including missing/worker loop, delayed result, peer state, adapter delegation and actual completion.health-check-session-cron-stamp.test.py: 22;session-cron-config-drift.test.py: 30;proactive-loop-skill-budget.test.py: 7; scheduler import-without-plistlib: 7; scheduler writer golden: 3; task filename delegation: all pass.ruff check .passes. Installed legacy Ruff cannot parse current config; it was left unchanged.Canonical documentation and catalog verification date updated in
docs/proactive-loop-rationale.md; source map regenerated. No documentation-navigation change.Disruption and mitigation
Worst case: existing Codex hosts without the canonical core-owned main-loop now report unhealthy; that is the missing-driver diagnosis, not an automatic repair or startup refusal. Deliberate parking still requires both an explicit disabled marker and an impossible schedule. The global legacy marker is intentionally not migrated because it can represent ordinary conversations or another host. New readers allow bounded first-proof installation grace; invalid records warn immediately. No schedules, credentials or production data are changed by the fix. No full repository test-suite claim; CI remains required.