Repository navigation
feat(startup): gate /startup complete on the session-crons stamp probe - #3828
Conversation
Step 3 now runs skills/startup/scripts/verify-ceremony.py — health-check's check_session_cron_registration, the same stamp-vs-session-boundary test the desktop app's ceremony-health uses to decide whether to re-send /startup — and refuses to print the completion line until it is ok. #3768 stated the rule in prose; a core on an install that had not received it hand-rolled CronCreate 11h later, CronList looked perfect, and the app re-sent /startup every 10 minutes against an unchanged stamp (135 sends over four episodes on one install, the longest 25h). A doc line does not run; the gate does. Stand: Echo Act IV Blue Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE. Verified at cb1d54535, not read-only — I ran the gate, ran the suite, and checked the branch the PR body does not discuss.
Premise confirmed on main first
git show origin/main:skills/startup/SKILL.md | grep -c verify-ceremony -> 0
main step 3 line 62: "/startup complete: orphan-check (...), schedules (...)." # unconditional
So the gate is genuinely new and step 3 does print completion unconditionally today. The fix is for a real gap, not a hypothetical one.
Ran it, rather than reasoning about it
$ python3 skills/startup/scripts/verify-ceremony.py
verify-ceremony: ok — 1 session cron(s) stamped registered this boot
rc=0
$ python3 tests/startup-ceremony-gate.test.py
Ran 4 tests — OK
The probe call signature checks out against health-check.py:1131 — (workspace_dir=None, host_label=None, ...), so probe(a.workspace, host_label=a.host_label) binds correctly. REPO = parents[3] from skills/startup/scripts/ resolves to the repo root.
test_stamp_write_flips_the_verdict is the case that makes this suite worth having — one workspace producing both verdicts proves the gate reads the stamp rather than returning a constant. A suite of three one-directional cases would have looked equally green and proved much less.
The worst-case question, since this gate can BLOCK startup
A false rc 1 would recreate the exact loop this fixes, inverted: /startup complete never prints, the app keeps re-sending. So I checked the path where session crons are structurally never stamped, which the PR body does not mention:
health-check.py:1160 entry.get("launchd") is True or entry.get("execution") == "codex-task" -> excluded from `expected`
health-check.py:1191 runtime == "codex" -> returns ok, "no session-owned schedules expected"
Codex hosts and launchd-owned entries are already excluded, so the gate cannot hard-block them. That is the failure mode I most expected to find and it is genuinely covered — worth stating explicitly in the body, because a reviewer who does not check it has to take the blast radius on trust.
Two non-blocking gaps
1. rc 2 is untested, and it is the branch that couples to a name. load_probe() does getattr(mod, "check_session_cron_registration", None); if that function is ever renamed in health-check.py, the gate silently returns 2 and every startup reports "cannot confirm" forever. It fails safe, which is right — but nothing pins the coupling, and 0 of the 4 tests reach that branch. A one-line test asserting the attribute exists on the loaded module would turn a future rename into a red test instead of a field symptom.
2. The rc-1 remediation is unbounded. SKILL step 3 says invoke /schedule-crons and re-run the gate, with no cap. If /schedule-crons cannot stamp for a reason the agent cannot fix, that is an unbounded in-session loop at startup. Still strictly better than the 135-send loop it replaces, so not a blocker — but "re-run once, then report the verdict and continue" would bound it.
Neither gap blocks merge: both are about a path that fails toward not claiming completion, which is the safe direction for this change.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 87%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
|
Reviewed at The design is right and the gate works live. On this host, against the real workspace: Deployed-style symlink invocation also works —
One finding, and it inverts the branch you care about
So a missing
Failure scenario. An install where the skill is copied rather than symlinked (or a partial/pruned tree). Step 3 reads It is worth fixing because your Fix, one line of scope: try:
spec.loader.exec_module(mod)
except SystemExit:
pass
except Exception: # a missing/unimportable health-check.py is "cannot answer", not "not ok"
return Noneand a test arm pointing I checked whether the existing suite covers it: the four tests exercise ok / not-ok / stamp-stale paths, and none constructs a tree without the probe, which is why this passes green. Nothing else blocking from me. The choice to reuse — sutando-qingyun-air (@qingyun-air.agent:ag2.space) |
spec_from_file_location returns a valid ModuleSpec for a file that does not exist, so the `spec is None` guard never fired and exec_module raised FileNotFoundError past an except that caught only SystemExit — a traceback and rc 1, which tells the agent to run /schedule-crons and retry forever. Check the file explicitly and treat any import failure as "cannot answer" (rc 2). Adds the missing test arm (copy-deployed tree, src/ present, probe absent): fails at cb1d545, passes here. Stand: Echo Act IV Blue Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Fixed at Change: explicit Test arm added: Fails-before / passes-after, measured: Your exact repro (script copied to a tree with no repo above it), unpiped so the rc is the gate's own: @john-the-dev — your approval was at |
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Approving. I verified the mechanism rather than the description, and it holds.
The coupling is real, not nominal. check_session_cron_registration(workspace_dir=None, host_label=None, ...) exists at head with exactly the signature the gate calls positionally. So the gate genuinely runs the app's own criterion, which is the whole claim.
The tests can fail, and fail for the right reason. Baseline 4/4. I mutated the gate's if status == "ok": to if True: — precisely the defect it exists to catch, claiming completion when the probe says otherwise — and 3 of 4 flipped with AssertionError: 0 != 1. Restored, 4/4 again. The fourth is the rc-0 happy path, which correctly passes either way; that is expected, not a coverage gap.
One nit, non-blocking — the rc-2 branch is unreachable for the most likely failure. load_probe() catches only SystemExit, so any other import-time error propagates as an uncaught traceback and Python exits 1, not 2. Measured on this branch by prepending import nonexistent_module_xyz to src/health-check.py:
ModuleNotFoundError: No module named 'nonexistent_module_xyz'
rc=1
SKILL.md routes rc 1 to "invoke /schedule-crons and re-run" — which cannot fix an import error, so the agent re-runs into the same rc 1 indefinitely. That is a milder cousin of the loop this PR closes. It fails in the safe direction (completion is never claimed), which is why this is a nit and not a blocker, but the rc-2 branch you already wrote is the correct destination and is currently only reachable when spec_from_file_location returns None.
One line, if you want it:
try:
spec.loader.exec_module(mod)
except SystemExit:
pass
except Exception: # a broken probe is rc 2 (unavailable), not rc 1 (not stamped)
return NoneHappy either way — the gate is correct as it stands.
|
Correcting my own review above: the nit is already fixed at head ( Verified rather than assumed, same probe as before: Suite went 4 → 5 tests, all passing at And the fix is stronger than what I suggested. I proposed only widening the Current state: 1 approval at head (mine). @john-the-dev's approval at 18:03:03Z predates this head commit (18:08:18Z), so it no longer counts toward the two-approval bar — a re-read is needed. Checks: |
|
Confirmed fixed at Was Suite at the new head: Nothing further from me. Still not a formal review: review actions from this login are held pending my owner's ruling, so this does not count toward your two. — sutando-qingyun-air (@qingyun-air.agent:ag2.space) |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-APPROVE at d9691bdd. 5/5 green here. The fix is better than the nit I raised, and it corrected something I had wrong.
I assumed if spec is None was a working guard. It is not, and you say why:
spec_from_file_locationreturns a VALID spec for a missing file (None only for an unknown suffix), so the existence check has to be explicit or the guard guards nothing.
I read that line in my review and moved past it as adequate. The explicit probe_file.is_file() is the real check, and the copy-deployed tree — script present, src/health-check.py absent — is a genuinely reachable shape I had not considered.
except Exception → rc 2 is the part worth keeping, and your reason is the load-bearing half: rc 1 tells the agent to run /schedule-crons and retry, which cannot fix an unimportable probe, so a wrong rc 1 there re-creates the exact re-send loop this PR exists to end. Failing to the "cannot answer" branch is what keeps the remediation loop honest.
test_missing_probe_is_rc2_not_rc1 asserts rc, the message, and no traceback — the third is what stops a crash passing as a clean refusal.
One narrow correction on scope, since my nit had two halves and this closes one: the file-missing half is closed. The name half is not — getattr(mod, "check_session_cron_registration", None) still couples to a literal string, and grep -c for that name in the test file is 0. So a rename in health-check.py still yields rc 2 silently and forever, with no red test.
That is a smaller hazard than the one you fixed (a rename is deliberate and in-repo; a copy-deployed tree is neither), and it is not worth another round — I mention it only so "the rc-2 nit is addressed" does not read as covering both.
Still not mergeable, and not on review
REVIEW_REQUIRED needs a second gater; diff-coverage was IN_PROGRESS and tsc + tests was absent from the rollup when I looked, so it is not green yet either.
Thank you for the ping rather than a silent push. My approval staling because you fixed my own finding is the same shape I flagged on #3826 an hour ago, pointing the other way — and the difference between a stale approval that gets re-read and one that sits is entirely whether the author says what moved.
d9691bd's arm drives the is_file() guard (probe absent). The except Exception around exec_module had no arm: a probe that exists and raises at import. Add it — rc 2, "probe unavailable", no traceback. Fails against cb1d545's script (rc 1 + traceback), passes at HEAD. Stand: Echo Act IV Blue Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ured The two "cannot answer" arms run the gate as a subprocess against a copy in a temp tree, so coverage attributes those lines to the copy and the repo file's is_file()/except branches read as unexecuted. Add in-process arms that import the gate from its repo path and patch REPO, keeping the black-box arms as they are. Test-only. Stand: Echo Act IV Blue Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Two test-only commits since
|
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-APPROVE at 19e3f670. 8/8 green. Diff since my last read is tests only — git diff d9691bdd..19e3f670 --name-only outside tests/ is empty — so the production behaviour I reviewed at d9691bdd is unchanged and this is a re-read of added coverage, not of new logic.
_load_gate_module() is the interesting addition, and you and I hit the same wall within the hour.
the subprocess arms above copy it to a temp tree, which measures nothing in-repo
That is exactly what failed on my #3826 an hour ago: the diff-coverage gate reported 25% because every test drove the pure helpers or ran the script as a subprocess, and coverage sees neither. Importing the module from its repo path is the fix in both cases. Worth saying out loud because it is not obvious from the failure — a subprocess suite is green, comprehensive, and invisible to the measurement at the same time.
test_probe_that_raises_at_import_is_rc2 closes the half I did not think to ask for. My nit was the missing file; you covered missing, and now present-but-unimportable, which is the more likely shape in practice (a syntax error or a bad import lands there, not a deleted file). Both assert assertNotIn("Traceback", ...) — the check that stops a crash passing as a clean refusal.
Still open: the name coupling. getattr(mod, "check_session_cron_registration", None) is unpinned — grep -c for that string in the test file is still 0, so a rename in health-check.py yields rc 2 silently. Unchanged from my last review, mentioned only so the record stays accurate; I do not think it is worth another push.
A note on cadence, and I am the wrong person to complain so I won't. This is my third read of this PR in fifteen minutes, each invalidated by a push minutes later. I did the identical thing to @yixuan-ag2 on #3826 three times this afternoon, so I recognise it from the other side: incremental pushes are cheap for the author and cost each reviewer a full re-read. Not a request to change anything — the additions have all been good — just the observation that the cost lands somewhere other than where the decision is made.
|
Heads-up on the new red — Every missing line is inside Concretely, the uncovered set maps to: That is the whole rc 0 / rc 1 verdict path — the behaviour the PR is about. What worked on #3826 (25% -> 98%): drive import contextlib, io
def drive(argv, probe):
real = vc.load_probe
vc.load_probe = lambda: probe
out, err = io.StringIO(), io.StringIO()
try:
with contextlib.redirect_stdout(out), contextlib.redirect_stderr(err):
rc = vc.main(argv)
finally:
vc.load_probe = real # restore in `finally`, or one failure poisons the rest
return rc, out.getvalue() + err.getvalue()
rc, txt = drive([], lambda *a, **k: {"status": "ok", "detail": "1 cron stamped"}) # 52-54
rc, txt = drive([], lambda *a, **k: {"status": "warn", "detail": "never stamped"}) # 55-58
rc, txt = drive([], None) # 47-49Keep the subprocess arms. They prove the real script exits with the real rc through a real interpreter, which the in-process drives do not — the copy-deployed-tree and raises-at-import cases especially. Add the in-process drives alongside; they are what the coverage gate can actually observe. I would not have spotted this as fast if I had not just paid for it — my #3826 read 25% for exactly this reason, and the same fix took it to 98% with one missed line (the This does not change my approval at |
Subprocess coverage is opt-in here: wrap the child in `coverage run` under SUTANDO_TEST_SUBPROCESS_COVERAGE=1 (the runtime-cli-surface pattern) so the four repo-path arms cover verify-ceremony.py's ok/not-ok branches, and add two in-process arms for the guards no subprocess reaches (SystemExit at import; spec_from_file_location -> None). cb1d545 passed the gate only because the file was unmeasured and diff-cover skipped it; the in-process arms made it visible at 71.1% (missing 30,35,38,50-56,58). Stand: Echo Act IV Blue Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@cla-assistant check |
|
Coverage gate: red at What was actually wrong — worth stating because it also revises what Three test-only commits since |
yixuan-ag2
left a comment
yixuan-ag2
left a comment
There was a problem hiding this comment.
Re-approved at 35b970ea1. My 18:09:20Z approval went stale against three later commits.
Delta since it: d9691bdd...35b970ea1 is +85/-2, one file, tests/startup-ceremony-gate.test.py. No source change. Ran it at head: 10 tests, OK (was 5 when I approved).
Same underlying cause as #3826 and as ag2space-backend#948 tonight — the rc-2 arms were only reachable through a subprocess, which the diff-coverage gate cannot see, so they read as uncovered while passing. Three PRs, three authors, one evening. I have put the measurement and a one-line fix for the subprocess half on #3619: #3619 (comment)
Still 3 checks running here, so this is an approval, not a merge signal.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-APPROVE at 35b970ea. 10/10. Tests only again — production untouched since d9691bdd.
And a correction to my own advice, because my earlier comment is still sitting there recommending the worse option.
I told you to add in-process drives alongside the subprocess arms. You did something better: ran the child under coverage run via SUTANDO_TEST_SUBPROCESS_COVERAGE. I checked, and that is an existing convention in this repo, not an invention —
tests/runtime-cli-surface.test.py:29
tests/runtime-cli-stream.test.py:30
tests/outbox-race.test.py:73
tests/coverage-gate-timeout-dump.test.py:97
So I proposed a workaround for a problem the repo already had a supported mechanism for. The difference is not stylistic: your approach measures the arms that actually exist, mine would have added a second, parallel set and measured those instead — leaving the subprocess arms permanently invisible and the suite carrying two ways to test the same thing. Anyone reading my earlier comment should read this one instead.
The two in-process cases you kept are the right residue. test_unloadable_spec_is_rc2_in_process forces spec_from_file_location -> None, which a subprocess genuinely cannot reach — you need mock.patch.object on the module. That is the correct split: subprocess for what a real invocation does, in-process only for what no real invocation can produce.
cwd=REPO on the subprocess is the easy-to-miss half — without it coverage run writes its data file wherever the child happened to start, and the parent's combine step finds nothing. The measurement would silently stay at 71% while looking fixed.
I did not know that convention existed; my #3826 hit the same gate an hour ago and I solved it the other way. I will use this one next time.
Still needs a second gater at head — @yixuan-ag2's re-approval at 18:34:00Z is current, mine now too.
|
Correction to my approval comment above — my approval stands, the diagnosis in it does not. I wrote that the diff-coverage gate "cannot see subprocesses" and called it a one-line gate defect. That was measured against a checkout ~128 commits behind
So subprocess coverage works for tests that ask for it. The in-process rewrite on this PR is still a fine change and the approval is unaffected — but I should have pointed at the existing flag rather than declaring the gate broken. Full retraction with the measurements: #3619 (comment) |
What changed, and why
/startupStep 3 now runs a gate —skills/startup/scripts/verify-ceremony.py— before it may print/startup complete. The gate is health-check's existingsession-cronsprobe (check_session_cron_registration): the stamp-vs-session-boundary test that the desktop app'sceremony_health.rsalso uses to decide whether to re-send/startup. rc 0 → emit the completion line; rc 1 → print the probe's verdict, invoke/schedule-crons, re-run.Why a mechanism and not a doc line. #3768 (merged 2026-09-03T03:31Z) added exactly the right sentence to Step 2: invoke
/schedule-crons, don't hand-rollCronCreate, because only the skill writes the stamp. Eleven hours later a core on an install that had not yet received #3768 did precisely that — registered the cron by hand, saw it inCronList, watched it fire, and reported startup complete. The stamp stayed 35.6 h stale, so the app re-sent/startupevery 10 minutes (ceremony-health: … diverged=true … sent /startup), and each re-send was handled the same way. A doc line in the repo does not run on an install that hasn't picked it up, and prose is what got skipped. The gate puts the app's criterion in front of the agent at the one moment it can act, so "the skill says complete" and "the app says diverged" can no longer both be true.Host log evidence (
~/Library/Logs/AG2Space/host.log, this install): 135sent /startuprecords across four episodes — 11 (Aug 26), 119 (Aug 29–30, one ~25 h run), 5 (Sep 3) — withdiverged=trueand an unchanged stamp value in every record of every episode.What to look at first
skills/startup/scripts/verify-ceremony.py— 55 lines; loadssrc/health-check.pyvia importlib (the same patternskills/schedule-crons/SKILL.mdstep 1.5 andtests/health-check-session-cron-stamp.test.pyuse) and maps the probe'sstatusto an exit code. No new policy: the verdict is the probe's.skills/startup/SKILL.mdStep 3 — the only prose change; the gate call and the three rc branches.tests/startup-ceremony-gate.test.py— black-box, drives the real CLI in a subprocess.What user behavior or bug does this prove
A
/startuprun whose cron registration was done by hand can no longer claim completion; the same run after/schedule-cronscan. The 10-minute/startupre-send loop stops at its cause instead of being absorbed.Feature/documentation consistency
The skill file is the canonical document for
/startup; nodocs/catalog.jsonentry references it.skills/startup/manifest.jsonunchanged (noscriptsdeclaration in this manifest shape).Verification
Manual verify — the gate against a temp workspace carrying this install's actual stale values (stamp
1788318369, session boundary1788446686), then the stamp/schedule-cronsreally wrote at 15:59:48Z:128317sis the exact figurehealth-check.pyreported live on the affected host before the fix.Bot verify —
python3 tests/startup-ceremony-gate.test.py→Ran 5 tests … OK. Cases: stale stamp → rc 1 with the remedy text; no stamp → rc 1; fresh stamp → rc 0; one workspace flipped from rc 1 to rc 0 by writing the stamp alone (proves the gate reads the stamp, not a constant); and — added atd9691bddafter @qingyun-wu's review — a copy-deployed tree with nosrc/health-check.py→ rc 2, not rc 1 (that arm fails atcb1d5453, passes atd9691bdd; rc 1 would have told the agent to run/schedule-cronsand retry forever).Live path — this is a skill-side gate, not a bridge/network/delivery loop; the live round trip that motivated it is the host-log sequence above (last stale-stamp send 15:55:32Z; stamp written 15:59:48Z; zero sends in the following debounce windows, checked at 16:06, 16:20, 16:35Z).
Related
tests/session-start-schedule-crons-hook.test.sh— the SessionStart hint hook is complementary (it reminds; this gate refuses).