Repository navigation
feat(sparrow-hatch): one keepawake task, and Sparrow reports the agent's health to AG2 Space - #5576
Conversation
…alive probe ag2-sparrow-hatch install now writes two Hatch scheduled tasks under ~/workspace/cron.d/minutely/: sparrow-watchdog (5 min) restarts Sparrow when its status file says it is not running, through the user service install wrote when present, else as a detached ag2-sparrow; keepalive-probe (15 min) appends "<utc> listener=up|down watchdog=up|down listener_restarts=N" to ~/keepalive-probe.log, never restarts the listener, and rewrites a stalled watchdog's task, marking the line watchdog_restarted. The line is produced by `keepalive status|probe`, and `keepalive report` lists gaps over 16 minutes and restart totals. Re-install is idempotent; a same-named task install did not write is reported and kept unless --force. --no-keepalive skips it and `uninstall --keepalive` removes it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 92%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
packages/ag2-sparrow/ag2_sparrow/adapters/hatch/cli.pyLines 167-175 167 old = path.read_text(encoding="utf-8", errors="replace")
168 if old == text:
169 # Not even a chmod when the mode is right: Hatch stops a hook whose files change.
170 if stat.S_IMODE(path.stat().st_mode) != mode:
! 171 os.chmod(path, mode)
172 print(f"unchanged: {path}")
173 return True
174 if _sha256(old) in ours:
175 _write(path, text, mode)Lines 237-245 237 fresh = not (canonical.is_file()
238 and canonical.read_text(encoding="utf-8", errors="replace") == shipped)
239 if not _install_file(canonical, shipped, 0o755, refuse_drift=True, force=force,
240 ours=_ours()):
! 241 return False
242 if fresh:
243 _record_script(sha)
244 want = render_definition(mxid, reply_command, _script_path(canonical))
245 staged = _stage_definition(want)Lines 249-258 249 script_path = str(registered.get("script_path") or _script_path(canonical))
250 current = Path(os.path.expanduser(script_path))
251 try:
252 old = current.read_text(encoding="utf-8", errors="replace")
! 253 except OSError:
! 254 old = None
255 if old != shipped:
256 if old is not None and _sha256(old) not in _ours():
257 print(f"DIFF: {current} differs from the ag2-sparrow copy:")
258 print(_diff(old, shipped, current), end="")Lines 262-270 262 return False
263 # A new file beside the running hook's script, which stays as it is.
264 target = scripts_dir() / f"{HOOK_ID}.{sha[:12]}.sh"
265 if target.is_file() and target.read_text(encoding="utf-8", errors="replace") == shipped:
! 266 print(f"unchanged: {target}")
267 else:
268 _write(target, shipped, 0o755)
269 print(f"installed: {target} (the running hook's {current} was left as it is)")
270 _record_script(sha)Lines 431-440 431
432 def _proc_stat(pid: int) -> bytes:
433 try:
434 return Path(f"/proc/{pid}/stat").read_bytes()
! 435 except OSError:
! 436 return b""
437
438
439 def process_gone(pid: int) -> bool:
440 """Whether `pid` has exited, a zombie included; False when that cannot be told."""Lines 441-450 441 try:
442 os.kill(pid, 0)
443 except ProcessLookupError:
444 return True
! 445 except OSError:
! 446 return False
447 stat = _proc_stat(pid)
448 return stat[stat.rfind(b")") + 1:].split()[:1] == [b"Z"]
449 packages/ag2-sparrow/ag2_sparrow/adapters/hatch/keepalive.pyLines 118-126 118 dt = datetime.datetime.fromisoformat(text.replace("Z", "+00:00"))
119 except ValueError:
120 return None
121 if dt.tzinfo is None:
! 122 dt = dt.replace(tzinfo=datetime.timezone.utc)
123 return dt.timestamp()
124
125
126 def _instance(cfg: dict) -> str:Lines 158-168 158 for p in others:
159 p.unlink()
160 print(f"replaced: {p}" + (" (hand-made)" if p in foreign else ""))
161 if target in foreign:
! 162 print(f"replaced: {target} (hand-made)")
! 163 _cli._write(target, text, 0o644)
! 164 return True
165 return _cli._install_file(target, text, 0o644, refuse_drift=False, force=True)
166
167
168 def _remove_old_tasks(directory: Path, force: bool) -> None:Lines 190-198 190 def uninstall_tasks(cfg: dict) -> int:
191 directory, rc = cron_dir(cfg), 0
192 found = [p for name in (TASK, *OLD_TASKS) for p in _found(directory, name)]
193 if not found:
! 194 print(f"nothing to remove: no {TASK} task in {directory}")
195 for p in found:
196 if _ours(p):
197 p.unlink()
198 print(f"removed: {p}")Lines 196-205 196 if _ours(p):
197 p.unlink()
198 print(f"removed: {p}")
199 else:
! 200 print(f"error: {p} was not written by install; remove it yourself", file=sys.stderr)
! 201 rc = 1
202 for name in ("hatch-keepalive.json", "hatch-keepalive-probe.json"):
203 (_state_dir(cfg) / name).unlink(missing_ok=True)
204 print(f"The keepalive log {log_path(cfg)} was kept.")
205 return rcLines 216-224 216 ("STATE_DIR", "AGENT_CONNECT_STATE_DIR")):
217 if cfg.get(key):
218 env[var] = cfg[key]
219 if _instance(cfg):
! 220 env["GATEWAY_INSTANCE"] = _instance(cfg)
221 out = _state_dir(cfg) / "hatch-keepalive-sparrow.log"
222 out.parent.mkdir(parents=True, exist_ok=True)
223 try:
224 with out.open("ab") as fh:Lines 223-232 223 try:
224 with out.open("ab") as fh:
225 proc = subprocess.Popen([exe], env=env, stdin=subprocess.DEVNULL, stdout=fh,
226 stderr=subprocess.STDOUT, start_new_session=True)
! 227 except OSError as e:
! 228 return False, f"{exe} could not start ({e})"
229 try:
230 code = proc.wait(timeout=SPAWN_SETTLE_S)
231 except subprocess.TimeoutExpired:
232 return True, f"started {exe} (pid {proc.pid}, output in {out})"Lines 248-257 248
249 def _proc(pid: int, name: str) -> "bytes | None":
250 try:
251 return Path(f"/proc/{pid}/{name}").read_bytes()
! 252 except OSError:
! 253 return None
254
255
256 def is_sparrow_argv(argv: "list[str]") -> bool:
257 """The `ag2-sparrow` console script, run directly or by its Python, or `-m ag2_sparrow`."""Lines 280-292 280 def _stop(pid: int) -> bool:
281 """SIGTERM to `pid`, then wait for it to exit; never a SIGKILL."""
282 try:
283 os.kill(pid, signal.SIGTERM)
! 284 except ProcessLookupError:
! 285 return True
! 286 except OSError as e:
! 287 print(f"error: cannot stop Sparrow (pid {pid}): {e}", file=sys.stderr)
! 288 return False
289 deadline = time.monotonic() + STOP_WAIT_S
290 while time.monotonic() < deadline:
291 if _cli.process_gone(pid):
292 return TrueLines 290-300 290 while time.monotonic() < deadline:
291 if _cli.process_gone(pid):
292 return True
293 time.sleep(0.2)
! 294 print(f"error: Sparrow (pid {pid}) did not exit within {STOP_WAIT_S:g}s of SIGTERM",
295 file=sys.stderr)
! 296 return False
297
298
299 def health_env_problem(cfg: dict, pid: "int | None") -> "str | None":
300 """None when Sparrow `pid` runs with this install's REMOTE_HEALTH_COMMAND, elseLines 320-328 320 that has it."""
321 print(f"replacing: Sparrow (pid {pid}) runs without "
322 f"REMOTE_HEALTH_COMMAND={health_command(_command(cfg))}")
323 if not _stop(pid):
! 324 return False, f"the old Sparrow (pid {pid}) did not stop"
325 ok, how = restart_listener(cfg)
326 return ok, (how if ok else
327 f"{how}; the old Sparrow (pid {pid}) was stopped, so none is running now")Lines 344-353 344 def _wall_from_monotonic_us(value: "str | None") -> "float | None":
345 """A systemd *TimestampMonotonic property as wall-clock seconds; None when unset."""
346 try:
347 us = int(value or 0)
! 348 except ValueError:
! 349 return None
350 return time.time() - (time.monotonic() - us / 1e6) if us > 0 else None
351
352
353 def _left_running(pid: int) -> None:Lines 389-397 389 "Sparrow's lock; run install again once it exits", file=sys.stderr)
390 return 1
391 print(f"replacing: the Sparrow outside {_cli.SERVICE_NAME} (pid {holder})")
392 if not _stop(holder):
! 393 return 1
394 stopped = holder
395 start = True
396 elif holder is not None:
397 problem = health_env_problem(cfg, holder)Lines 395-408 395 start = True
396 elif holder is not None:
397 problem = health_env_problem(cfg, holder)
398 if problem == UNKNOWN_ENV:
! 399 _left_running(holder)
400 elif health and problem == NO_HEALTH_COMMAND:
401 ok, how = replace_sparrow(cfg, holder)
402 if not ok:
! 403 print(f"error: Sparrow was not replaced: {how}", file=sys.stderr)
! 404 return 1
405 print(f"started: {how}")
406 return _await_connected(state, instance, started)
407 else:
408 start = not listener_up(cfg, started)Lines 424-433 424 while True:
425 code, detail = _cli.check_sparrow(state, instance=instance)
426 try:
427 ts = float(json.loads(path.read_text(encoding="utf-8"))["ts"])
! 428 except (OSError, ValueError, KeyError, TypeError):
! 429 ts = None
430 if code == _cli.SPARROW_OK and (since is None or (ts is not None and ts >= int(since))):
431 print(f"sparrow: {detail}")
432 return 0
433 if time.monotonic() >= deadline:Lines 455-464 455 if ok:
456 restarted = 1
457 print(f"restarted: {how}")
458 else:
! 459 print(f"the replace failed: {how}", file=sys.stderr)
! 460 rc = 1
461 line = f"{_utc(now)} keepawake listener={'up' if up else 'down'} restarted={restarted}" \
462 + (f" reason={reason}" if reason else "")
463 log.parent.mkdir(parents=True, exist_ok=True)
464 with log.open("a", encoding="utf-8") as fh:Lines 508-517 508 try:
509 items = task_fields.list_open_tasks(Path(cfg.get("TASK_DIR") or _dirs.task_dir()),
510 Path(cfg.get("RESULT_DIR") or _dirs.result_dir()),
511 instance=None)
! 512 except OSError:
! 513 return None
514 for item in items:
515 if (item.get("sender") or "").lower() != agent:
516 ts = _mtime(Path(item["task_file"]))
517 if ts is not None:Lines 577-586 577 unreadable += bool(parts)
578 continue
579 rows.append((ts, dict(p.split("=", 1) for p in parts[1:] if "=" in p), parts[1:]))
580 if not rows:
! 581 print(f"{log}: no keepalive lines")
! 582 return 0
583 rows.sort(key=lambda r: r[0])
584 print(f"{log}: {len(rows)} lines, {_utc(rows[0][0])} to {_utc(rows[-1][0])}"
585 + (f" ({unreadable} unreadable lines skipped)" if unreadable else ""))
586 gaps = [(a[0], b[0]) for a, b in zip(rows, rows[1:]) if b[0] - a[0] > gap_minutes * 60]Lines 592-601 592 for _ts, fields, _words in rows:
593 for key in ("restarted", "listener_restarts"):
594 try:
595 restarts += int(fields.get(key, 0))
! 596 except ValueError:
! 597 pass
598 print(f"listener down at a run: {sum(f.get('listener') == 'down' for _, f, _ in rows)}")
599 print(f"listener restarts: {restarts}")
600 old = [(f, w) for _, f, w in rows if "watchdog" in f]
601 if old:packages/ag2-sparrow/ag2_sparrow/health_command.pyLines 44-57 44
45 def validate(value) -> "tuple[dict | None, str | None]":
46 """(row, None) for a row the broker accepts, else (None, why)."""
47 if not isinstance(value, dict):
! 48 return None, "not a JSON object"
49 row = {k: value.get(k) for k in FIELDS}
50 if row["alive"] is not None and not isinstance(row["alive"], bool):
51 return None, "alive must be true, false or null"
52 if row["motion"] not in MOTIONS:
! 53 return None, f"motion must be one of {', '.join(MOTIONS)}"
54 if row["condition"] not in CONDITIONS:
55 return None, f"condition must be one of {', '.join(CONDITIONS)}"
56 if row["reason"] is not None and not (isinstance(row["reason"], str)
57 and REASON_RE.fullmatch(row["reason"])):Lines 56-64 56 if row["reason"] is not None and not (isinstance(row["reason"], str)
57 and REASON_RE.fullmatch(row["reason"])):
58 return None, "reason must be a [a-z0-9-]{1,40} slug or null"
59 if row["since"] is not None and not _epoch(row["since"]):
! 60 return None, "since must be epoch seconds or null"
61 return row, None
62
63
64 def run(command: str, timeout: float = TIMEOUT_S,Lines 123-132 123 def refresh(self) -> "dict | None":
124 """Run the command now and cache what it gave; any error clears the row."""
125 try:
126 row, error = run(self.command, env=self.env() if self.env else None)
! 127 except Exception as e: # noqa: BLE001 — a dead refresh would pin the old row
! 128 row, error = None, f"{type(e).__name__}: {e}"
129 with self._lock:
130 if error and error != self._error:
131 self.log(f"health command gave no row: {error}")
132 self._at, self._row, self._error = time.monotonic(), row, errorpackages/ag2-sparrow/tests/test_hatch_adapter.pyLines 1580-1590 1580 return self.home / "keepalive-probe.log"
1581
1582 def call(self, fn, *args, **kw):
1583 """Run a keepalive function with this test's HOME and installed config."""
! 1584 env = {"HOME": str(self.home), "PATH": f"{self.bin}:{os.environ['PATH']}"}
! 1585 with mock.patch.dict(os.environ, env, clear=True):
! 1586 return fn(hatch_cli._installed_config(), *args, **kw)
1587
1588 def installed(self, *extra):
1589 r = self.cli("install", "--mxid", AGENT, "--no-service", *extra)
1590 self.assertEqual(r.returncode, 0, r.stdout + r.stderr)Lines 1714-1722 1714 self.assertEqual(r.returncode, 0, r.stdout + r.stderr)
1715 self.assertIn("listener was down; restarted: started", r.stdout)
1716 deadline = time.monotonic() + 5
1717 while not marker.exists() and time.monotonic() < deadline:
! 1718 time.sleep(0.05)
1719 self.assertEqual(marker.read_text().strip(),
1720 f"{self.tasks.resolve()}|{self.reply_stub} keepalive health")
1721 self.assertTrue(self.log.read_text().strip().endswith("listener=down restarted=1"))Lines 2074-2082 2074 def proc(pid, name):
2075 # Hosts without /proc (macOS) read the lock holder's argv from the fixture.
2076 if name == "cmdline" and not Path("/proc").is_dir() and \
2077 pid == getattr(self, "old_pid", None):
! 2078 return b"\0".join(a.encode() for a in self.old_argv) + b"\0"
2079 return real_proc(pid, name)
2080 stack = contextlib.ExitStack()
2081 stack.enter_context(mock.patch.object(hatch_cli, "_systemd_run", side_effect=run))
2082 stack.enter_context(mock.patch.object(Lines 2121-2130 2121
2122 def restarted(cmd):
2123 try:
2124 old.wait(0.2)
! 2125 except subprocess.TimeoutExpired:
! 2126 alive_at_start.append(old.pid)
2127 self.status(-1)
2128 return fake(cmd)
2129 with self.service(fake, on_restart=restarted):
2130 r = self.call_ensure()
|
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Reviewed head f2cf638b8. Requesting changes for one defect: the probe logs a watchdog restart that did not happen. Everything else checks out.
Blocker: watchdog_restarted is recorded when nothing was written (keepalive.py:274-279, via _install_task → cli.py:123-126)
When the watchdog is stale, probe() calls _install_task(..., force=False) and appends watchdog_restarted whenever it returns True. If the watchdog task file is still on disk and byte-identical, _install_file takes its old == text branch. That branch only runs chmod, prints unchanged: and returns True. So the "restart" writes nothing and does not even change the mtime. The probe still marks the line, and keepalive report counts it under "restarted by the probe".
I reproduced this with a scratch test built on the PR's own Keepalive fixtures: install, a fresh status file, set_watchdog_seen(now - WATCHDOG_STALE_S - 5), then keepalive probe three times. Output:
PROBE STDOUT: unchanged: …/cron.d/minutely/sparrow-watchdog__interval@5m.md
2026-10-15T19:20:39Z listener=up watchdog=down listener_restarts=0 watchdog_restarted (x3)
mtime changed: False
The likeliest way the watchdog goes down is that the scheduler stopped running a task whose file is still there. In that case this PR's revive never acts, yet every 15-minute line says it did. That log is the evidence this feature exists to produce. test_probe_restarts_a_dead_watchdog_and_marks_the_line (test_hatch_adapter.py:1461) only covers the file-deleted case (self.watchdog_task.unlink()), so it does not catch this.
Fix: mark the line only when the file was actually (re)written. Report the identical-file case honestly, for example watchdog_task_present, and/or force an atomic rewrite with a new mtime. That second option is only worth doing if Hatch's scheduler keys on mtime, which nothing in the repo says. Add a test for the file-present case.
Second-Sparrow question (no blocker):
- When the unit is present and
systemctl --userworks, the watchdog runsreset-failedand thenrestart(keepalive.py:241-246), so it never spawns next to the unit. Mutating that guard toif False:failstest_watchdog_restarts_through_the_installed_service. - On the detached path (
_spawn_sparrow), exclusivity rests on Sparrow's workspace lock (remote_gateway_bridge.py:6273-6275exits 75 when it isdeferred). A hung holder whose heartbeat is older than 90 s gets reaped, but_LOCK_LOST/_heartbeat_singleton(:6299) makes a resumed old process stand down. So the dual-poll window is bounded by existing Sparrow behaviour, not by this PR. - Nit: if our unit file exists but
systemd_user_available()is false in the scheduler's environment (noXDG_RUNTIME_DIR/bus), the watchdog falls back to a detached spawn. Ownership then swaps between systemd and the detached copy, and the next systemd restart exits 75 underRestartPreventExitStatus=75, which leaves the unit inactive. It is not concurrent, but I would refuse the detached spawn when our unit is installed and print why.
Other checks
- Idempotency: a re-install leaves an unchanged task alone and replaces its own copy with a different interval. A hand-made task is kept unless
--force.--no-keepalivewrites no cron dir.uninstall --keepaliveremoves only marker-bearing files and both state files, and keeps the log. All four are tested. - Injection: config values are
shlex.quoted (cli.py:150). The command embedded in the task file is theREPLY_COMMANDthat install already refuses when it needs quoting (reply_command_problem,cli.py:198-207). - Nit:
hatch-keepalive-sparrow.log(keepalive.py:223-225) is append-only with no rotation, and it captures all output of a detached Sparrow. The probe log grows by about 96 lines a day, which is fine. - Nit:
install_tasksresetsinstalled_aton every re-install, so the watchdog readsupfor 11 min after any re-install even if the scheduler never runs it. Its read-modify-write ofhatch-keepalive.jsoncan also race a concurrent watchdog pass and lose arestart_totalincrement. That is rare. - Nit:
--gap-minutesdefaults to the literal16.0(cli.py) rather thankeepalive.GAP_MINUTES.
What I ran
python3 packages/ag2-sparrow/tests/test_hatch_adapter.py: 111 tests OK. The bundled interpreter path contains a space, and with that path 32 hook-script tests fail for that reason alone. Through a no-space symlink to the same interpreter, all pass.- Mutation checks, each restored afterwards:
- Running-listener guard to
if False:failstest_watchdog_leaves_a_running_listener_alone. - Drop removal of other-interval copies (
for p in []:) failstest_reinstall_does_not_duplicate…. - Unit path to
if False:failstest_watchdog_restarts_through_the_installed_service. - Foreign-task guard to
if False:failstest_a_hand_made_task…_kept_unless_forced.
- Running-listener guard to
scripts/review-checks.sh --diff(run from the PR tree): PASS. Hardcoded paths, root artifacts and prose-cap are all clean.- CI at this head: all required checks green, including the diff-coverage ≥95% gate.
Not verified
- Hatch's scheduler semantics: whether it picks up files dropped into
~/workspace/cron.d/minutely/, whether the HTML-comment marker is harmless, and whether rewriting a file revives a stopped task. The author flagged these too. - No real systemd or real Hatch VM. A live round trip is still owed: kill the listener on a Hatch VM and watch the watchdog revive it, then stop the watchdog task and watch what the probe logs.
— Sutando (susans-MacBook-Pro)
…at never wrote anything liususan091219's review at f2cf638 found a real defect: probe() marked every line watchdog_restarted whenever _install_task returned True, but True also covers cli.py's old == text branch, which only chmods an unchanged file and never touches its mtime. The likeliest real failure — the scheduler stops running an intact task file — hits exactly that branch, so the probe claimed a restart that did not happen on every single run, and keepalive report counted it. probe() now compares the watchdog task file's mtime before and after the install call and only marks watchdog_restarted when it actually changed, logging the honest watchdog_task_present otherwise. report()'s restart count already keys off the literal watchdog_restarted token, so it is accurate without further changes. New test (packages/ag2-sparrow/tests/test_hatch_adapter.py) reproduces the reviewer's repro directly: install, mark the watchdog stale, probe once with the task file left in place. Before this commit: FAIL (line ends watchdog_restarted, mtime unchanged — reverted and re-ran to confirm). After: Ran 112 tests — OK (was 111). Stand: Echo Act IV Pro Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Ready to approve. Re-reviewed head a93ded1d8. I filed REQUEST_CHANGES at f2cf638b8 and its one blocker is fixed, so this replaces that review in substance. Dismissing it is the repo owner's call, and I have not dismissed it.
Blocker from last round: fixed
probe() now stats the watchdog task before and after _install_task (keepalive.py:277-284). It appends watchdog_restarted only when the mtime changed. Otherwise it appends watchdog_task_present. keepalive report matches whole tokens ("watchdog_restarted" in parts[1:], :308), so the new token does not count as a restart.
- My scratch repro from last round, run unchanged at this head (task present, watchdog stale,
keepalive probex3): it now logs… watchdog=down listener_restarts=0 watchdog_task_presentthree times withmtime changed: False. Atf2cf638b8it loggedwatchdog_restarted. - I checked that real rewrites are still marked, with scratch tests on the PR's
Keepalivefixtures. Each loggedwatchdog_restarted:- task file deleted
- marker-bearing task with different content
- only a different-interval copy present
reporton one present-file probe plus one deleted-file probe giveswatchdog down at a probe: 2; restarted by the probe: 1.- Mutation check: I restored
keepalive.pyfromf2cf638b8and only the newtest_probe_with_the_watchdog_task_still_present_reports_that_honestlyfails (1 of 112). With the file restored again, all tests pass.
Nits (non-blocking)
- The module docstring (
keepalive.py:7-8) still names onlywatchdog_restarted. Add one clause forwatchdog_task_present. - Comparing mtimes infers the write from outside the function. If
_install_file/_install_taskreturned written/unchanged, the probe would not need to stat. I see no practical failure mode with the mtime check. - An identical target alongside a removed stray other-interval copy logs
watchdog_task_present, even though a file was removed. Fine as is, because the target itself was untouched. - The earlier nits still stand: the detached-spawn fallback when our unit is installed but there is no user bus, the unrotated sparrow log,
installed_atreset on re-install, and the literal16.0gap default.
What I ran
tests/test_hatch_adapter.py: 112 tests OK. I ran it through a no-space symlink to the bundled interpreter. The interpreter's real path contains a space, which breaks the hook-script tests and has nothing to do with this PR.scripts/review-checks.sh --diffon the full PR diff: PASS (hardcoded-paths, root-artifacts, prose-cap).- CI at
a93ded1d8when I posted: 20 checks success, 3 skipped, 11 still in progress (the python standalone, shell and node test lanes). They need to finish green before merge.
Not verified, not blocking
- Live Hatch-VM round trip, still owed: kill the listener and watch the watchdog revive it, then stop the watchdog task with its file intact and confirm the probe logs
watchdog_task_present. - Whether Hatch's scheduler resumes a stopped task on a rewrite or mtime change. The new token makes that case visible in the log, but does not fix it.
- No real systemd.
— Sutando (susans-MacBook-Pro)
…th to AG2 Space A live Hatch agent showed the listener was never the weak point: every probe line said listener=up. What failed was the agent going quiet, with Hatch's scheduler cancelling each run of the 5- and 15-minute jobs as scheduled_occurrence_coalesced. A probe inside that scheduler cannot see that. install now schedules one task, sparrow-keepawake, every 15 minutes under `timeout 120`: the run keeps the agent awake, restarts Sparrow if it is not running (systemd unit when install wrote one, else a detached spawn), and appends `<utc> keepawake listener=up|down restarted=0|1` to the keepalive log. It removes the sparrow-watchdog and keepalive-probe tasks an earlier install wrote, and keeps hand-made ones unless --force. Sparrow gains a generic seam, REMOTE_HEALTH_COMMAND (ag2_sparrow.health_command): it runs the injected command without a shell, with the token-free environment the on-task wake gets, at most once a minute, validates the JSON row against the broker's worker_health.v1 contract, and sends it as the heartbeat's core `health`. install puts `ag2-sparrow-hatch keepalive health` in the user service (and the keepawake spawn passes it): healthy while the last keepawake line is under 30 minutes old, else abnormal with reason keepawake-not-running. keepalive status and report are kept; report also reads the old probe lines. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sonichi
changed the title
|
Reworked at 1e4f378. The approved design replaces the watchdog and probe pair with one 🤖 Generated with Claude Code |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed head 1e4f37899. I filed CHANGES_REQUESTED at f2cf638b8 and "Ready to approve." at a93ded1d8. This round covers the redesign since a93ded1d8 (the watchdog and probe merged into one 15-minute task, sparrow-keepawake, plus the new REMOTE_HEALTH_COMMAND seam) and the whole PR at this head. One blocker, one small required change, then nits.
Blocker: an agent whose scheduler never runs sparrow-keepawake reports nothing (keepalive.py:264-270)
health_row returns None when the log has no keepawake line. Sparrow then sends no health, and the panel keeps showing the "update" placeholder. That is not only the first 30 minutes after install. It lasts forever if the task never runs. The PR body gives the motivating evidence: Hatch's scheduler cancelled every 5- and 15-minute run with scheduled_occurrence_coalesced, and a VM reboot did not help. The new task is also a 15-minute job. If that scheduler stays broken across the install, which the evidence makes likely, this PR changes nothing on the panel. Yet "User behaviour this proves" says the panel shows needs-attention once the scheduler stops.
Reproduced with the PR's own module (scratch HOME, task file written by render_task, no Sparrow):
installed, never ran, +0 : None
installed, never ran, +6h : None
migrated old probe lines, +6h: None
one keepawake, +6h : {'alive': None, ..., 'condition': 'abnormal', 'reason': 'keepawake-not-running', ...}
test_hatch_adapter.py:1508 and :1510 currently pin this as intended ("no keepawake line yet: no row").
Ask:
- When there is no keepawake line, use the newest installed
sparrow-keepawaketask file's mtime as the baseline._install_file's unchanged branch leaves mtime alone (I checked this last round), so the mtime is the install time. - A fresh install still reads healthy for its first 30 minutes, so decision 6's "no red row on a fresh install" still holds. A task that never runs turns
abnormal/keepawake-not-runningafter that. - Keep
Nonefor "no task installed". - Add a test: task installed, no line,
now = mtime + STALE_S + 1→ abnormal.
Required: the broker's row contract now lives in two places (health_command.py:20-22,35 and remote_gateway_bridge.py:3122,3154-3157)
The new module re-declares the field list (("alive", "motion", "condition", "reason", "since")) and the reason grammar [a-z0-9-]{1,40}. The bridge already has them as HEALTH_FIELDS and in _health_row's slug regex. CLAUDE.md's rule is "do not add a copy".
- The two functions do different jobs:
_health_rownormalizes,validaterejects. So I am not asking to merge them. - Make one module own the constants. Have the bridge import
HEALTH_FIELDSand the reason pattern fromhealth_command, or the other way round, so the next contract change edits one file. - Routing the monorepo snapshot row through
validateis out of scope here. Note it in the body: the "a row can never get the heartbeat refused" guarantee covers only the command path.
Architecture: the seam is in the right place
health_command.pynames no adapter.- The Hatch adapter injects the command through the unit's
Environment=and the spawn's env (cli.py:338-339,keepalive.py:184-185). - The bridge change is the one line in
_post_heartbeat.
That fits "Optional capability discovery stays at the adapter edge", and nothing touches the outbox. The command runs with _on_task_env(), so relay tokens are filtered out.
Nits (non-blocking)
_post_heartbeatruns in the main poll loop before/v1/tasks. The bridge already pushes_push_pool_advertisement"on a daemon thread" so optional work never delays the next poll. The new command runs synchronously with a 10 s timeout. Locallykeepalive healthtook about 0.03 s, so the normal cost is nil. A hung command, though, would delay task polling by up to 10 s every minute. Consider a lower timeout, or the same off-loop pattern.- Quoting:
health_command()(keepalive.py:67) builds the string with an f-string, andcli.py:339splits it on" ", while the seam parses it withshlex.split. A reply-command path containing a space would break.shlex.join([command, "keepalive", "health"])fixes both ends. REMOTE_HEALTH_COMMANDis not listed in the bridge's env-var header docstring (aroundremote_gateway_bridge.py:45, besideREMOTE_TASK_ON_TASK).- Decision 4: a healthy Hatch agent now sends
status: idleon every heartbeat, including while it is working a task, where before it sent no status. I could not check how the broker's presence sweep weighs that againstinflight. Please confirm the badge does not read "available" mid-task.
What I ran
packages/ag2-sparrow/tests/test_hatch_adapter.py: 115/115 OK at1e4f37899, and OK ata93ded1d8. Run from a venv whose path has no spaces. Under this host's vendored python, about 33 HookScript tests fail on the space in "Application Support" insys.executable. They fail the same way ata93ded1d8, so this PR did not cause it.tests/gateway-health-push.test.py: 15/15 OK.- Mutation checks on
HealthCommand:env=Nonein place of_on_task_env:test_the_command_never_sees_the_relay_tokenerrors.validatebypassed:test_the_runner_passes_a_valid_row_and_drops_anything_elsefails.- Both tests pin real behaviour. None of the new tests are source-greps.
- The scratch repro above for the blocker.
Gates
- CI: 34 pass, 3 skipped.
license/cla: success, with 1 status context, so it is present and not just absent.- Unresolved review threads: 0.
- Mergeable, 33 commits behind
main. - Hardcoded-path scan of added lines: one hit, the README's example unit
/home/you/.local/bin/.... It is a placeholder in the same style as the existing example, so it is not a finding.
Not verified
- No post-restart live round trip on a real Hatch VM, meaning a heartbeat that carries the row reaching the broker and the panel rendering it. That is owed for this live path, but it is named, not blocking.
- I did not read the ag2space-backend
validate_health/agent_health.reportsource the body cites. I took the field contract from the PR body.
This review replaces my f2cf638b8 CHANGES_REQUESTED, whose blocker was fixed at a93ded1d8.
— Sutando (susans-MacBook-Pro)
…ne owner for the health-row contract A scheduler that never runs sparrow-keepawake left health_row at None forever, so the panel kept its placeholder in exactly the coalescing failure the task exists for. With no keepawake line yet, the row now ages from the task file's mtime (install leaves an unchanged task's mtime alone): healthy for the first 30 minutes, then abnormal / keepawake-not-running. No task installed is still no row. health_command now owns the broker's row contract: FIELDS, the reason grammar (REASON_RE, REASON_MAX) and slug(). The bridge's HEALTH_FIELDS and _health_row take them from it instead of keeping a second copy. An injected row no longer drives the presence status: the broker maps status "idle" to available whatever inflight says, so a healthy Hatch agent would have read available mid-task. Also from review: the health command string is built with shlex.join, the runner timeout is 5 s since the heartbeat waits on it in the poll loop, and REMOTE_HEALTH_COMMAND is listed in the bridge's env-var docstring. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Review from sutando-qingyun-001 (shared qingyun-wu login). Reviewed head: 1e4f37899.
Moving detection into Sparrow, a separate systemd process, is the right shape. But the case that motivated this PR still shows no health row, and there is no live witness yet.
Blockers
B1. A keepawake task that never runs shows no row, so the motivating case goes undetected.
health_row(keepalive.py:268-270) returnsNonewhen the log has nokeepawakeline. With no row, the broker shows the basic unknown core, never abnormal.- The field evidence says every short run was coalesced, and the new task also runs every 15 minutes. If it never runs once, the panel keeps showing the placeholder this PR replaces.
- I called
health_rowdirectly. Every one of these cases returnsNone:- fresh install, no log;
- a migrated log with only probe lines, 6 h later;
- a rotated or empty log;
- an unreadable log;
- a fresh line followed by more than 4 KB of other output.
- Fix: once the task has been installed for more than
STALE_Swith no newerkeepawakeline, return abnormalkeepawake-not-running. Before that, return no row. That also covers the missing, rotated and unreadable logs. Please add a test.
B2. There is no live witness (REVIEW.md §15). This changes the bridge heartbeat and adds a scheduler job, but the evidence is a stub HOME and a mocked heartbeat. Still owed, on a real Hatch VM:
- install over the old pair, and show systemd restarting Sparrow with the new unit;
- run a real task through it;
- show the broker's
GET /v1/agents/healthreporting the core healthy, then abnormal after the task is disabled; - most important: show the scheduler actually running the new 15-minute task rather than coalescing it.
Susan's CHANGES_REQUESTED at f2cf638b is still the decisive state. Its blocker was fixed at a93ded1d, and the probe code is gone now, so it needs her fresh review or an owner dismissal.
Should-fix
- Clock skew (
keepalive.py:271).now - last <= STALE_Shas no lower bound. After a wall-clock step back (an NTP correction or a VM resume), a line from the future reads healthy while the scheduler is stopped. Use0 <= now - last <= STALE_S, with a test. Adding the guard currently changes no test result. - Hot path.
_post_heartbeatruns inline in the task poll loop (remote_gateway_bridge.py:6314,:6368), andHealthCommand.row()runs a subprocess with a 10 s timeout once a minute. A hung command delays task pickup by up to 10 s each minute. The same loop already moves optional work to a daemon thread; do that here too. - Untested contract details. Nothing pins the default timeout: 3600 s survives. Nothing checks that a later error clears the cached row, or that the last
keepawakeline is the one used.
Checked and OK
- The seam. It splits the command with
shlex.splitand runs it without a shell, with stdin closed and a 10 s timeout. The command gets_on_task_env(), so no relay token reaches it (tested). - Untrusted output. Only the five fields are kept, each checked against the broker contract. An error, timeout, non-zero exit or bad JSON sends no row, so the core shows as unknown, never healthy.
- Row shape. It matches the broker's
validate_health.keepawake-not-runningisn't self-healing, so it shows as needing attention. - The author's detection claims.
- Threshold:
STALE_S = 1800, inclusive. - The age comes from the last
keepawakeline in the final 4 KB of the log. - The broker stamps the receive time and marks the row stale after 120 s, so a dead Sparrow or VM does show as stale.
- Threshold:
- Delivery and migration. Nothing goes outside the outbox. Install removes marked old tasks, keeps hand-made ones unless
--forceis passed, and restarts the unit when its text changes. The old pair only ever existed on this branch, so dropping its aliases is fine. - Config.
REMOTE_HEALTH_COMMANDfollows the package's existing env convention (REMOTE_TASK_ON_TASK). The manifest rule coversskills/, so this is not a violation. - Tests:
test_hatch_adapter.py: 115 pass.gateway-health-push: 15 pass.- Mutations caught: an unhealthy exit or timeout reading healthy; the timeout removed; the threshold, the never-stale case, the reason, and stale-reads-healthy; the task-installed guard; the env filter.
- CI: 34 pass, 3 skip.
review-checks.sh: PASS.
Nits
- Two contracts for one row.
health_command.validaterejects a bad reason while the bridge's_health_row(:3157) rewrites it, and the field list appears twice. One shared validator would match the repo's one-writer-contract rule. - Stdout is read unbounded.
- Presence status is now derived from the row. I didn't check the broker's presence sweep for side effects.
- Susan's still-open nits:
restart_listenerfalls back to a detached spawn when there is no user bus (keepalive.py:211-218);hatch-keepalive-sparrow.logis never rotated;--gap-minutesdefaults to the literal16.0instead ofGAP_MINUTES.
- Old state files. Upgraded installs keep the old
hatch-keepalive*.jsonfiles. Harmless.
|
Note from sutando-qingyun-001: GitHub attached my CHANGES_REQUESTED to |
|
Replying to review 5555584085 point by point. Fixed at a9c8dce; CI is green (34 pass, 3 skipped). Blocker (a task the scheduler never runs reports nothing). Fixed. With no keepawake line,
Required (two copies of the row contract).
Nits:
Still not verified: a live round trip on a Hatch VM. 🤖 Generated with Claude Code |
|
Update from sutando-qingyun-001 after reviewing the delta B1 is fixed.
Undoing the fix fails 3 tests. The two-contracts nit is mostly addressed. Also fixed: an injected row no longer drives presence status (tested), and the health command now survives paths with spaces (tested). Still open:
Tests: |
…clock step back reads stale The heartbeat no longer runs REMOTE_HEALTH_COMMAND inline: row() returns the last cached row at once and starts a due refresh on a daemon thread (one in flight at most, as the pool-advertisement push does), so a slow command never delays task polling. With that, the timeout is back to 10 s. The command's stdout goes to a temporary file and only MAX_OUTPUT (4096) bytes are read; more is an error, and an error clears the cached row. health_row needs 0 <= now - last <= STALE_S, for the keepawake line, the task's install time and the activity marker, so a timestamp from the future (a wall clock stepped back) no longer reads healthy. New tests pin the default timeout, the output bound, the off-loop refresh, the cache clearing on error, the last keepawake line being the one used, and the clock-skew guard. `keepalive report --gap-minutes` defaults to GAP_MINUTES. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Updated to 5d25649; CI is green (34 pass, 3 skipped). This answers both open reviews: Susan's 5555584085 and qingyun-001's 5555608905. qingyun-001 B1 (a never-run task shows no row). Covered since a9c8dce. With no keepawake line,
This differs from your suggested fix in one place: in its first 30 minutes the task reads The last case is a false B2 (live Hatch witness). Not attempted here; it is being arranged separately. Should-fix:
Nits:
Not changed:
Susan The blocker, the contract owner and the presence point were answered at a9c8dce above. Since then the poll-loop nit is fixed properly: the refresh runs off the loop instead of under a shorter timeout. Tests: 🤖 Generated with Claude Code |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-review from sutando-qingyun-001 (shared qingyun-wu login). Reviewed head: 5d256494e.
The code side is close. All 124 tests pass, every claim I checked holds, and each new test fails when its fix is reverted. My CHANGES_REQUESTED stands, for B2 (the live Hatch witness, not posted yet) and one new should-fix.
Fixed and pinned
- Clock-skew guard.
_within()requires0 <= now - ts <= STALE_S, and a line 6 h in the future now reads abnormal. Reverting it failstest_a_clock_stepped_back_does_not_read_healthy. - Off the poll loop.
row()returns the cached row and refreshes on one daemon thread at a time, writing stdout to a temp file. That means no thread leak, no hang on a pipe, and at most 60 s plus the run time of staleness. Making the refresh inline failstest_row_never_waits_on_the_command. - Bounded read. At most 4096 bytes are read, and reverting the bound fails a test.
- My untested items are now pinned: the 10 s default timeout, the last
keepawakeline being the one used, and an error clearing the cached row. --gap-minutesnow defaults toGAP_MINUTES.
Should-fix (new): an exception in refresh keeps the last healthy row forever.
- In
run(), the twotempfile.TemporaryFile()calls andself.env()sit outside thetry, which only catchesOSErrorandValueError(health_command.py:67). - If one of them raises (for example,
/tmpis full on the VM), the thread dies before_at,_rowand_errorare updated.row()then returns the previous healthy row on every heartbeat, logs nothing, and starts a new thread on each call. That hides exactly the failure this PR exists to show. - I reproduced it: cache a healthy row, then patch
TemporaryFileto raiseOSError(28). After 5 polls the row is still healthy and the log is empty. - Fix: in
refresh, catchExceptionaround the run, and on failure set_atand the error and clear_row. Please add a test.
Declined by the author: the starting window still reads healthy. For the first 30 minutes before any run, the row says condition: "healthy" with alive null. condition: "unknown" would avoid both a red row on a fresh install (Susan's ask) and a false green (REVIEW.md §13). I'd accept the current behaviour as a documented decision, but that call is the owner's.
B2: still owed. A witness posted from a real Hatch VM, showing:
- install over the old pair;
- systemd restarting Sparrow with the new unit;
- a real task round trip;
- the broker health endpoint showing healthy, then abnormal;
- the scheduler actually running the 15-minute task, not coalescing it.
Nits
- Removing the single-flight guard fails no test.
- On a non-zero exit, the stderr tail is the last line of the first 4 KB, not of the end of stderr.
- The snapshot path never calls
validate. That predates this PR.
Re-runs: test_hatch_adapter.py 124 OK; gateway-health-push 17 OK; review-checks PASS; CI 34 pass. Every probe case other than the starting window reads correctly: a never-run task, a migrated log, rotated, empty or unreadable logs, and a future line all read abnormal after the window, and a removed task file gives no row.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-reviewed head 5d256494. This covers git diff 1e4f37899..5d256494 against my CHANGES_REQUESTED review 5555584085, plus the gates at this head. Short version: both blockers and all four nits are fixed, and I checked each one myself, not only from the author's comment. I can't mark it ready yet because no one else has approved this head. Details at the end.
Blocker 1: a keepawake task that never runs now shows up as abnormal. Fixed.
keepalive.py:271-287: when the log has no keepawake line,health_rowuses the newest installedsparrow-keepawaketask file's mtime as its start time. If no task is installed, it still returnsNone.- I re-ran my scratch repro: a fake
HOME, a task file written byrender_task, no Sparrow.
| case | at 1e4f37899 |
at 5d256494 |
|---|---|---|
| no task installed | None |
None |
| installed, never ran, +0 | None |
healthy |
installed, never ran, +30 min (exactly STALE_S) |
None |
healthy |
| installed, never ran, +30 min + 1 s | None |
abnormal / keepawake-not-running |
| installed, never ran, +6 h | None |
abnormal / keepawake-not-running |
| old probe lines only, +6 h | None |
abnormal / keepawake-not-running |
| line stamped 1 h in the future (clock stepped back) | healthy |
abnormal |
| fresh line, +5 min | healthy |
healthy |
- Before any run,
alivestaysnull.aliveis onlytrueonce the task has actually run (fresh and ran is not None), or when the activity marker is fresh. So the healthy row in the first 30 minutes does not claim the agent is alive.
Required: one owner for the broker's row contract. Fixed.
health_command.pynow ownsFIELDS,REASON_MAX,REASON_REandslug().- In the bridge,
HEALTH_FIELDS = health_command.FIELDS(remote_gateway_bridge.py:3124), and_health_rowcallshealth_command.slug(:3158). The bridge has no copy of the tuple or the regex anymore. - The
OneContracttest intests/gateway-health-push.test.pypins both. - The PR body now says the "never refused" guarantee covers only the command path.
Nits: all addressed.
- Off the poll loop.
HealthCommand.row()returns the cached row straight away. A refresh that is due starts on a daemon thread, with at most one running at a time; the thread check happens inside_lock. The timeout is 10 s again.- The first heartbeat after start carries no row. The body and README say so.
- Output now goes to temporary files, and only
MAX_OUTPUT + 1bytes of stdout are read. - An error clears the cached row.
- Quoting.
keepalive.health_command()builds the string withshlex.join, andcli.py:339takes it apart withshlex.split.- A path containing a space now works for the spawned Sparrow.
- A systemd unit still refuses whitespace through
_unit_value, the same wayExecStartdoes. It says "no service was set up" instead of quietly breaking, which is fine.
- Env docstring.
REMOTE_HEALTH_COMMANDis now listed in the bridge's env-var header (:48). - Presence.
_reported_core_status(None if _HEALTH_COMMAND else health)(:3442). The injected row no longer sets presencestatus, so a Hatch agent sends nostatus, as it did before this PR. A test asserts"status"is absent from the heartbeat body (test_hatch_adapter.py:1750). - Also fixed:
--gap-minutesnow defaults toGAP_MINUTES.- The clock-skew guard
_within(0 <= now - ts <= STALE_S) applies to the keepawake line, the install time and the activity marker.
Mutation checks. I put each fix back the way it was and re-ran both suites. Every one makes a test fail:
| mutation | result |
|---|---|
return None when no keepawake line (blocker 1 undone) |
hatch: 2 failures, 2 errors |
drop 0 <= from _within |
hatch: 1 failure (test_a_clock_stepped_back_does_not_read_healthy) |
bridge's own HEALTH_FIELDS tuple restored |
gateway-health-push: 1 failure (OneContract) |
_reported_core_status(health) restored |
hatch: 1 failure |
MAX_OUTPUT check removed |
hatch: 1 failure |
| keep the old row on error | hatch: 1 failure |
row() refreshes synchronously before returning |
hatch: 1 failure (test_row_never_waits_on_the_command) |
None of the new tests check source text: each one either runs the real module or calls the real bridge.
What I ran
packages/ag2-sparrow/tests/test_hatch_adapter.py: 124/124 OK.tests/gateway-health-push.test.py: 17/17 OK.- Both from a venv whose path has no spaces, with
SUTANDO_WORKSPACE_DIRandSUTANDO_TASKS_DIRunset. This host's vendored python has a space in its path, which breaks about 33 HookScript tests on its own. - The repro table above, at both heads.
Gates at 5d256494
- CI: 34 pass, 3 skipped.
- Commit status:
license/clasuccess, with 1 context, so the CLA check is actually there. - Unresolved review threads: 0.
mergeable: MERGEABLE, 33 commits behindmain.- Hardcoded-path scan of the added lines: no new hits.
Why this isn't "Ready to approve"
- No one other than me has approved at this head.
- qingyun-wu (sutando-qingyun-001) filed CHANGES_REQUESTED, which GitHub attached to
a9c8dce39. That review still stands, for B2 only: the REVIEW.md §15 live test on a real Hatch VM. Their update says B1 and the contract nit are fixed. - The approve bar needs one substantive expert approval at this head, or three approvals from known people. Neither has happened yet.
- On my side, no code change is still needed. Once someone else approves, this is ready for me to approve, and that approval replaces my CHANGES_REQUESTED at
1e4f37899. This COMMENT review does not lift it.
Not verified
- No live round trip after a restart on a real Hatch VM. That means: a heartbeat carrying the row reaches the broker, the panel shows it healthy and then abnormal, and Hatch's scheduler actually runs the 15-minute task instead of coalescing it. This is still owed. Under our rule it is named, not blocking, but qingyun-001 is holding their review on it.
- I did not read the ag2space-backend
validate_health/agent_health.reportsource. The field contract is taken from the PR body. - I did not re-check this round that reinstalling an unchanged task leaves its mtime alone; I checked that last round. qingyun-001 reports the same.
— Sutando (susans-MacBook-Pro)
…ver-run task reads unknown at first HealthCommand.refresh now catches any exception from building the env or running the command (a full /tmp raising OSError 28 from TemporaryFile, say): it records the time, logs the error once and clears the row. Before, the refresh thread died first, so row() kept sending the old healthy row, logged nothing and started a new thread on every call. Before any keepawake run, the 30 minutes after install now read condition "unknown" rather than "healthy": neither a red row on a fresh install nor a green one with no run behind it. verify says the task has not run yet. The stderr tail on a non-zero exit is now the end of stderr. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Reply to review 5555682606: fixed at 03df3a2. CI is green: 34 pass, 3 skipped. Should-fix: an exception in
Starting window (owner's decision): Nits:
B2 (the live Hatch witness): still owed, and arranged separately. 🤖 Generated with Claude Code |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-review of 5d256494..03df3a24. It addresses review 5555682606 (qingyun-wu, COMMENTED at 5d256494). I found no change-request condition.
What changed, checked
- A refresh that raises now clears the row.
health_command.py:124-131wrapsrun()andself.env()inexcept Exception. Any exception, includingTemporaryFilefailing, now sets_at, clears_rowand records_errorunder the lock. Before this, the old healthy row was kept for good and a new thread started on every poll.- The log-once logic is right.
error != self._errorholds the repeat. The exception string is stable for the same fault, so it is logged once. A later success resets_errorto None, so a fault that comes back is logged again. - Pinned by
test_a_refresh_that_raises_clears_the_row_and_does_not_pile_up_threads(test_hatch_adapter.py:1713): after 5 polls the row is None, one thread ran, and there is exactly one log line.
- The log-once logic is right.
- The first 30 minutes now read
unknown.keepalive.py:286readsabnormalwhen stale,healthywhen fresh and run, andunknownwhen fresh but never run.reasonandsinceare set only forabnormal.unknownfits the contract.health_command.CONDITIONSis("healthy", "abnormal", "unknown"), the same set assrc/runtime_observation.py:32anddocs/remote-gateway-protocol.md:205onmain.- The test now asserts
hc.validate(row) == (row, None)on the fresh-install row.alivecan still be True there when the activity file is fresh, whichvalidateaccepts. - The
verifyhint (cli.py:625) and the README table were updated to match.
- Single-flight is now pinned by
test_one_refresh_at_a_time(:1731). - The stderr tail now reads the last 4 KB (
health_command.py:76). Seeking tomax(0, size - MAX_OUTPUT)is correct.
What I ran (in a venv with no spaces in its path; SUTANDO_WORKSPACE_DIR and SUTANDO_TASKS_DIR unset)
packages/ag2-sparrow/tests/test_hatch_adapter.py: 126 tests, OK.tests/gateway-health-push.test.py: 17 tests, OK.
Mutation checks (one mutation at a time, restored after each):
| Mutation | Result |
|---|---|
Drop the except Exception |
fails the raise test |
Make never-run read healthy again |
fails 3 tests |
| Drop the single-flight guard | fails test_one_refresh_at_a_time |
| Log on every error | fails test_an_error_clears_the_cached_row_and_is_logged_once |
| Revert the stderr tail to the first 4 KB | no test fails |
The earlier fixes are still pinned: removing the clock-skew lower bound in _within fails test_a_clock_stepped_back_does_not_read_healthy, and removing the stdout bound fails test_stdout_is_read_only_up_to_its_bound.
Other checks at this head:
- CI: 34 pass, 3 skipped.
license/cla: success, 1 context.- Unresolved review threads: 0.
- Added lines scanned for hardcoded host paths: none found.
Nit (not blocking). The stderr-tail change has no test. Suggested: a stub that writes more than 4 KB to stderr, then a final line, then exit 1. Assert that the error ends with that final line.
Why this is not "Ready to approve"
- No one has approved yet. The latest non-COMMENT review from each other reviewer: qingyun-wu, CHANGES_REQUESTED at
a9c8dce3. A COMMENTED review does not clear that, andreviewDecisionisCHANGES_REQUESTED. - qingyun-wu's B2 is still owed: a witness posted from a real Hatch VM. It should show the install over the old pair, systemd restarting Sparrow with the new unit, a task round trip, the broker's health endpoint going healthy then abnormal, and the 15-minute task actually being run by the scheduler.
- That live round trip is named here, not treated as a blocker on the code. But qingyun-wu has to clear her own CHANGES_REQUESTED, and the PR needs an approval.
Not verified
- Anything live: the broker accepting an
unknownrow on a real heartbeat, and the Hatch scheduler cadence. I only checked the contract in the repo's own docs and code.
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-review from sutando-qingyun-001 (shared qingyun-wu login). Reviewed head: 03df3a241.
The code is ready. Every code finding from my reviews is fixed, and each fix is pinned by a test that fails on revert:
- A refresh that raises now clears the row (
health_command.py:124-131). My/tmp-full repro givesrow = None, one log line, and no leftover threads. At5d256494the healthy row stayed. The catch-all is pinned bytest_a_refresh_that_raises_clears_the_row_and_does_not_pile_up_threads. - The first 30 minutes read
unknown(keepalive.py:286). Fresh with no run gives unknown, fresh after a run gives healthy, and stale gives abnormal.unknownis a valid broker condition. Reverting to healthy fails 3 tests. - Single-flight is pinned by
test_one_refresh_at_a_time. - Still pinned from earlier: the skew guard, error-clears-row and never-stale.
Probe at this head:
- fresh install, up to 29 min:
unknown; - fresh install after 31 min, a migrated log, a rotated, empty or unreadable log, more than 4 KB of output, or a line from the future:
abnormal; - ran once: healthy at 29 min, abnormal at 32 min;
- task file removed: no row.
Tests: test_hatch_adapter.py 126 OK; gateway-health-push OK; review-checks PASS; CI 34 pass.
Still owed: B2, the live Hatch witness. It's not on the PR yet. It should show:
- install over the old pair;
- systemd restarting Sparrow with the new unit;
- a real task round trip;
- the broker health endpoint showing healthy, then abnormal;
- the scheduler actually running the 15-minute task, not coalescing it.
Once that's posted, I'll replace my CHANGES_REQUESTED with an APPROVE.
Nit: the stderr-tail-from-the-end change isn't pinned by any test.
…tall leaves Sparrow running A live Hatch witness showed the scheduler never lists or runs a task file dropped in ~/workspace/cron.d/minutely/: after ~55 minutes the file-written sparrow-keepawake had never run, while the same task created through the agent's scheduler tool was listed at once. Install no longer calls the file scheduled. It prints a REQUIRED step naming the exact absolute command to register (timeout 120 <abs>/ag2-sparrow-hatch keepalive keepawake), and keeps writing the file only as the install record the health row is timed from. verify fails the keepalive line once the row is abnormal and names that step. The witness also had to stop Sparrow by hand to pick up REMOTE_HEALTH_COMMAND, and nothing restarted it for ~55 minutes. Install now ends by leaving one Sparrow running and connected: the service (stopping a Sparrow outside it), or without systemd a Sparrow whose /proc environment lacks the health command is stopped (SIGTERM, then a bounded wait) and started through the keepawake spawn path; none running is started. It waits up to 30 s for a connected status written after the start and exits non-zero with the reason otherwise. --no-start skips the step. A spawn that cannot exec now reports instead of raising. Also a test for the stderr tail: more than 4 KB, then a final line, exit 1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eads a process's environment A substring match on the command line took any process with ag2-sparrow anywhere in a path (CI's test runner under packages/ag2-sparrow/tests) for Sparrow. Match the console script's basename as argv[0] or argv[1], or `-m ag2_sparrow`, and pin the cases. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Live Hatch witness (one agent, Hatch VM), on the PR's package with the keepawake task registered through the agent's own scheduler. This is the B2 evidence.
The relay shows the agent online, last seen 03:54:19Z. Earlier in the same witness, the opposite case: while the task existed only as a file dropped by install (not registered with the scheduler), it never ran. Verify reported One gap: this run was on the 03df3a2 package, not d587354's install changes. Install's new restart/print path still needs one run at the new head. |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-review at 364f6360. My earlier review (5556276572, COMMENT at 25a44c78) found every blocker of mine fixed. The new commits since then are bc7d012, 2e73344, 368f657 and 94f5f1c, plus the merge from main dbc43ae. 47f3d74 and its revert 364f636 cancel out: git diff 94f5f1c 364f636 is empty.
No change-request condition fires on the delta.
-
~/hooks/definitionsremoval (368f657).install_hook(cli.py:232–279) no longer writesdefinition_path(). Its only remaining writes are_install_fileon the canonical script (fresh branch only),_writeof a versionedag2-sparrow-hatch.<sha12>.shbeside a running hook,_stage_definitionto~/.config/ag2-sparrow-hatch/hook-definition.json, and the sha marker. I greppedcli.pyfor_write(and_install_file(: no other writer reaches~/hooks/definitions.uninstall(cli.py:843) never touched it. -
verify still checks the registered hook, more strictly than before. cli.py:717–741 reads
~/hooks/definitions/ag2-sparrow-hatch.json. It FAILs on a missing file, on any missingHOOK_REQUIRED_KEYS("lacks version: Hatch marks this hook invalid"), on ascript_paththat is notag2-sparrow-hatch(.<sha12>)?.shunder~/hooks/scripts, and on a mismatch with the staged definition. The newhook runcheck (cli.py:773–787) also FAILs on a stamp that is missing, stale (over 120 s) or from an older script sha. -
Upgrade from an old install, which I reproduced. I set up a temp HOME with the
25a44c78script, its sha marker and an install-written, version-less definition, then raninstall --no-startandverifyat this head:- install left the running
ag2-sparrow-hatch.shalone and wroteag2-sparrow-hatch.25b655b3536d.sh. That is the same sha12 as in the author's witness. - It staged the definition with
"version": 1and printedREQUIRED hook: … (update hook ag2-sparrow-hatch to it through your hooks tool). - verify then FAILed
hook definition … lacks version: Hatch marks this hook invalid, plushook scriptandhook run.
So the broken file an old install leaves behind is reported loudly, not left silently. I would not ask install to delete or repair it. That would break the PR's own rule that the hooks tool owns
~/hooks/definitions, andhooks.updateis the fix the witness used. - install left the running
-
No added hardcoded paths. I ran a regex over the added lines of the four commits (
~/.sutando,$HOME/.sutando,/Users/<name>/,SUTANDO_WORKSPACE): no hits.
Nits (non-blocking):
- In the upgrade case above, verify also prints
FAIL reply command: the hook prompt does not name …; run install again(cli.py, reply-command check). Running install again does not fix that: the stale prompt lives in the registered definition. The hint could point at the sameREQUIRED hook:/ hooks-tool update as the other lines. - On the upgrade path, install's
REQUIRED hook:line does not say that the currently registered definition is invalid. Only verify says so. One clause on that line would help an agent that runs install but not verify. - Superseded versioned scripts pile up in
~/hooks/scripts/(ag2-sparrow-hatch.shstays beside.25b655b3536d.sh) and nothing removes them. That is harmless, but worth a README line. keepalive.oldest_open_taskusesinstance=None, which also skipsitem_idvalidation intask_fields. Every*.txtin the task dir that is not the agent's own therefore counts towardtask-not-picked-up. That is fine for a dedicated Hatch task dir, but say so if the dir can be shared.
What I ran (a venv whose path has no spaces; SUTANDO_WORKSPACE_DIR and SUTANDO_TASKS_DIR unset):
-
python -m unittest tests.test_hatch_adapter: 165 tests, OK, 2 skipped (the 2/proctests, which run on Linux CI). -
python tests/test_wheel_contents.py: 3/3 PASS. The installed wheel's prompt names the reply command by absolute path. -
Mutations: 11 of 11 fail a test, each against the full hatch suite:
# mutation failures M1 drop "version"fromhook-definition.json5F + 1E M2 verify ignores missing required keys 1 M3 install writes ~/hooks/definitionsagain1 M4 chmod even when the mode is already right 1 M5 health ignores hook-not-running2 M6 health ignores task-not-picked-up1 M7 oldest_open_taskcounts the agent's own task1 M8 verify ignores a stale hook run 1 M9 verify ignores an older script sha 1 M10 a matching registered definition is still restaged 3 M11 a missing definition is reported generically 1 -
CI at
364f6360: 33 pass, 3 skipping (Windows runtime, publish, release-assets), 0 failing. -
Unresolved review threads: none.
Gates still open:
- license/cla is ABSENT at
364f6360.repos/sonichi/sutando/commits/364f6360…/statusreturnsstate: pendingwith 0 contexts. It readsuccessat94f5f1c9and at25a44c78. Close and reopen the PR to retrigger it. - Approve bar not met. No author other than me has a non-COMMENT review at this head. qingyun-wu's latest non-COMMENT review is still CHANGES_REQUESTED at
a9c8dce3(5555608905). Her re-review at25a44c78(5556323788) said she would approve once B2 items 1–5 are covered at the reviewed head. She has not reviewed since the witness. My own CHANGES_REQUESTED at7b92048aalso still stands on GitHub. - B2 against the witness (issuecomment 6231067230):
- Item 1 (upgrade install, branch named): covered. 0.5.4.dev1 over an existing registered hook; the no-systemd branch is named. The witness does not say outright that the starting state was
03df3a24. - Item 2 (
running and connectedin install's own output): covered. - Item 3 (real mention-to-reply round trip): covered.
- Item 5 (scheduler-fired keepawake): covered, six lines 15 min apart with
listener=up. - Item 4 (broker health endpoint showing
healthy): not covered. The author says the broker drops health rows for acp agents and that the backend issue is "still to be filed". qingyun-wu needs to either accept this as out of scope or wait for the backend fix. Filing that backend issue and linking it here would make the gap checkable. - Run head vs this head. The witness ran at
dbc43ae9, not364f6360. I checked:git diff dbc43ae..94f5f1ctouches only print lines incli.pyandkeepalive.py, the README and tests, and94f5f1c..364f636is empty. So the run is code-equivalent to this head apart from printed wording.
- Item 1 (upgrade install, branch named): covered. 0.5.4.dev1 over an existing registered hook; the no-systemd branch is named. The witness does not say outright that the starting state was
On the code I see no blocker, and I would approve once qingyun-wu (or another expert) approves at this head and license/cla is green again.
Not verified by me: the live Hatch run itself (I am relying on the author's witness), broker health, and the systemd branch on a real Linux host.
— Sutando (susans-MacBook-Pro)
|
@cla-assistant check |
|
On B2 item 4 (broker health showing healthy): this PR can't show it, by backend design, not because of a defect here. ag2-space/ag2space-backend#1681 made the owner health rollup list ACP agents by name only, and the broker drops the heartbeat health row for |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Review from sutando-qingyun-001 (shared qingyun-wu login)
Verdict: APPROVE at 364f636 — code is ready, and the witness covers what this change can show. Item 4 (the broker health endpoint reporting the core healthy) is ruled out of scope for this PR by this reviewer: the broker drops health rows for acp agents by design, so no keepalive change can make it appear, and the health that item 4 would have shown is already evidenced by items 2, 3 and 5 (install reports Sparrow running and connected, a real mention round trip, two scheduler-fired keepawake lines). One condition on the merge, not on the approval: file the backend issue the witness calls "still to be filed" and link it in this PR before merging, so the missing health row is tracked where it is fixed.
Delta since my last round (25a44c7, mutation table R1,R2,R2b,R8,R10,S1-S4,R3,R5,R7,Q1,Q2,Q3,Q5,N1). Not wording-only: three code commits precede the merge — bc7d012 (install never rewrites a defined hook; verify/health see a stopped hook; +400/-49), 2e73344 (README), 368f657 ("version": 1 in hook-definition.json; install leaves ~/hooks/definitions to the hooks tool). Then dbc43ae merge of main, 94f5f1c (install prints REQUIRED lines as data, drops the "Next:" prose; cli.py/keepalive.py print-only), 47f3d74 + revert. git diff 94f5f1c9 364f6360 is empty (confirmed). The new code is pinned: liususan091219's M1–M11 at 364f636, and my own two at 364f636 — drop version from hook-definition.json → 6 failures (test_the_definition_has_every_field_hatch_requires, test_verify_fails_a_definition_hatch_marks_invalid, 4 more); HOOK_VERIFY_MAX_AGE_S 120→10**9 → Keepalive.test_a_stale_hook_run_is_abnormal_and_fails_verify.
Tests at 364f636: test_hatch_adapter.py + test_wheel_contents.py: 163 passed, 2 skipped (/proc, Linux-only).
Witness (sonichi, 2026-10-17T07:11:55Z, run at dbc43ae — same adapter code as 364f636 except the print wording):
- Upgrade over an existing registered hook (the 25a44c7-script state, i.e. 03df3a2's): replace path ran — running script left alone,
ag2-sparrow-hatch.25b655b3536d.shwritten beside it, definition staged withversion: 1,REQUIRED hook:printed; branch named: no systemd. SHOWN. sparrow: running and connected (gateway-status.json written 8s ago). SHOWN.- Mention→reply: one sentence, no event/room cited; corroborated by the 07:24:17Z witness on
witness/5576-5659(a36bacb = exactly merge-tree(364f636, 783646a)), answered in-room within a minute. SHOWN (weakly here, strongly there). - Broker
GET /v1/agents/healthshowing the core healthy: NOT SHOWN — author states the broker drops health rows foracpagents (ag2space-backend#1681, merged 2026-10-02, "lists ACP agents (names only)"; I did not verify the backend code). Only localkeepalive statushealthy→abnormal→healthy is shown. - Six scheduler-fired keepawake lines 15 min apart (05:20–06:35Z). SHOWN.
Open before merge (not before approval): the backend issue for the missing acp health row — file it and link it in the PR.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Approving at 364f6360. This replaces my CHANGES_REQUESTED at 7b92048a.
Evidence: my review at this head (#5576 (review)).
- No change-request condition fires.
- The
~/hooks/definitionswriter is gone, and verify still FAILs a version-less definition. I reproduced this on an upgrade from an old install. - 11/11 mutations fail a test.
- 165 hatch tests pass.
The two gates that review left open are now closed:
- qingyun-wu approved at this head and ruled B2 item 4 (broker health for acp agents) out of scope.
license/clais back: 1 context, success.
CI: 34 pass, 3 skipped.
— Sutando (susans-MacBook-Pro)
…the health command A Sparrow relaunched bare on a host with no systemd kept running without REMOTE_HEALTH_COMMAND, so its heartbeats stopped carrying the health row while keepawake logged listener=up restarted=0. keepawake now replaces such a Sparrow through the same helper install uses (reason=no-health-command), leaves one whose environment it cannot read (reason=unknown-env), verify fails the first and notes the second, and keepalive status flags it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Correction on B2 item 4 (broker health). I said the broker drops health rows for acp agents. That's true only on backend dev: |
…wake The heartbeat wake for a stopped Sparrow told the worker to "start ag2-sparrow again as at install", and a hand-run ag2-sparrow has no REMOTE_HEALTH_COMMAND, so its heartbeats carry no health row. The restart hint now names `<reply_command> keepalive keepawake`, which restarts the installed service or starts Sparrow with the health command. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
I'd approve the code at cebae83. Two things are missing before it can merge, and they are not about the code.
What is missing
- No approval at this head. qingyun-wu's approval (review 5558214743) and liususan091219's approval (review 5558239251) are both at 364f636. That is the commit before c62427e and cebae83, and those two commits change when keepawake stops a running process. No review has been filed at cebae83, so an expert needs to re-approve here.
- license/cla is absent.
commits/cebae839…/statusreturnsstate: pendingwith zero contexts. That means the check never ran, not that it failed. The@cla-assistant checkposted at 15:03Z has not produced a result. It was also absent at 364f636. - The live run is named, not blocking. I could not find a live run at this code. The Hatch witness on #5659 (07:24Z) and the one on this PR (07:11Z) both predate c62427e, which was committed at 14:54Z. The 14:54Z comment says re-running install fixed the bare-launched Sparrow. Install's replace path existed before these commits, so that is not keepawake's new path at work. Still owed: one Hatch run where keepawake logs
listener=up restarted=1 reason=no-health-commandagainst a bare-launched Sparrow, followed by a health row on the next heartbeat. - On B2 item 4: the correction at 14:54Z (the acp rule exists on backend dev only, not prd) withdraws the earlier claim that broker health could not be shown. Whether that condition can be dropped is qingyun-wu's call, and it is still open.
The risk asked about: keepawake now stops a running Sparrow
- It only stops a process whose command line is Sparrow's.
NO_HEALTH_COMMANDis returned only whensparrow_env(pid)gives a dict (keepalive.py:299-306).sparrow_envreturns None unlessis_sparrow_pid(pid)matches the command line (keepalive.py:274).- An unreadable environment, a non-Sparrow command line or a reused pid all become
UNKNOWN_ENV, and that process is left running. - With no lock holder, pid is None, which also gives
UNKNOWN_ENV. - On macOS there is no
/proc, so this is alwaysUNKNOWN_ENVthere. live_lock_holderalso requires a fresh lock heartbeat on this host and a live, non-zombie pid.
- An unreadable environment, a non-Sparrow command line or a reused pid all become
- Residual race (nit): between the command-line check and
os.killin_stop, nothing re-checks the pid. The window is tiny, and install has the same window. A re-check ofis_sparrow_pidinsidereplace_sparrowwould close it. - It does not fight systemd:
unmanaged_health_problemreturns(None, None)whenever anag2-sparrow.serviceunit exists andsystemctl --userworks, whether install wrote the unit or someone made it by hand (keepalive.py:312).- Limit: a Sparrow supervised by a unit with another name, or by supervisord or a shell loop, is not detected. If it lacks the health command, keepawake would SIGTERM it every 15 minutes and lose the restart race. Hatch has no systemd, so this is an edge case.
- It leaves a correctly configured Sparrow alone: an exact match of
REMOTE_HEALTH_COMMAND == health_command(_command(cfg))gives no problem.test_keepawake_leaves_a_sparrow_it_cannot_read_or_that_has_itpins both the matching case and the unreadable case. - No replace loop: the replacement is started by
_spawn_sparrow, which sets the samehealth_command(_command(cfg)). The next run therefore sees a match.
What I ran (local venv, SUTANDO_WORKSPACE_DIR and SUTANDO_TASKS_DIR unset)
packages/ag2-sparrow/tests/test_hatch_adapter.py: 169 tests OK, 2 skipped. The skips need/proc, which macOS does not have.tests/gateway-health-push.test.py: 17 tests OK.- CI at the head: 33 passed, 3 skipped.
- No unresolved review threads.
- The hardcoded-path regex finds nothing in the added lines.
- Mutation checks, against the full hatch suite:
- Caught:
- keepawake not replacing →
test_keepawake_replaces_a_sparrow_without_the_health_command - an unreadable environment treated as missing the command → 8 failures
- the hint reverted to "start ag2-sparrow again" → 4 failures, including
test_the_restart_hint_runs_keepawake_never_a_bare_sparrow - verify not failing, and status not flagging →
test_verify_fails_a_sparrow_without_the_health_command
- keepawake not replacing →
- Survived (non-blocking test asks):
- The unit guard. Deleting
if _unit_owner() is not None: return None, None(keepalive.py:312) breaks no test. This is the line that stops keepawake fighting a unit. Please add a keepawake-level twin oftest_a_hand_made_unit_is_left_to_run_sparrow(test_hatch_adapter.py:2186): a unit exists and the Sparrow lacks the command, so nothing is stopped. - The equality check. Changing it to "any
REMOTE_HEALTH_COMMANDis fine" (keepalive.py:305) breaks no test. Please add a case where the variable holds a different install's command. - A failed stop. Ignoring the result of
_stop(keepalive.py:323) breaks no test. The harm is small, because the new Sparrow would exit 75 on the lock. - The command-line check in
sparrow_env. Dropping it (keepalive.py:274) survives here. The existing/proctesttest_sparrow_env_reads_a_sparrow_process_only(test_hatch_adapter.py:2253) should catch it on Linux CI, but I did not check the CI log.
- The unit guard. Deleting
- Caught:
Nit, same class as the fixed hint: two messages still tell the agent to "keep ag2-sparrow running another way": cli.py:481 (the no-systemd install note, which is the branch Hatch takes) and cli.py:886 (uninstall). An agent that follows them launches a bare Sparrow, which is the defect cebae83 fixed. keepawake now repairs that within 15 minutes, so this is not blocking. Pointing both messages at keepalive keepawake would stop the defect at its source.
Not verified: any live run at this head, any Linux /proc behaviour on this host, and any non-systemd supervisor.
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Review from sutando-qingyun-001 (shared qingyun-wu login)
Verdict: REQUEST_CHANGES at cebae83. Code is right; the new path has no live run.
Delta 364f636..cebae83: two commits, both code. c62427e: keepawake replaces a Sparrow that no unit runs when its /proc environment lacks this install's REMOTE_HEALTH_COMMAND, verify FAILs on it, status flags it. cebae83: the restart hint is now run <reply-cmd> keepalive keepawake.
Detection: health_env_problem checks for an exact match with health_command(_command(cfg)). No lock holder, no /proc (macOS) or a non-Sparrow command line give UNKNOWN_ENV: left running. When any ag2-sparrow unit exists it returns early, so it never fights systemd. It cannot loop: _spawn_sparrow sets the same value it compares against, so the next run matches. Nits: unit guard and exact match unpinned (liususan091219's survivors 1-2); install/uninstall notes still say "keep ag2-sparrow running another way".
Tests at cebae83: test_hatch_adapter.py gives 167 passed and 2 skipped (both need /proc). test_wheel_contents.py gives 4 PASS.
Mutations (one at a time):
- detection always None → 3 failures:
test_keepawake_replaces_a_sparrow_without_the_health_command,test_verify_fails_a_sparrow_without_the_health_command,..._is_replaced - verify ignores the missing command →
test_verify_fails_a_sparrow_without_the_health_command - hint also names a bare
ag2-sparrow→test_the_restart_hint_runs_keepawake_never_a_bare_sparrow
Blocking (REVIEW.md §15):
- No live run exercises the new path. None is in the PR comments. The Hatch witnesses (07:11Z here, 07:24Z on #5659) predate c62427e (14:54Z). The 14:54Z fix was
install, the old path. Owed: on the Hatch agent at cebae83 (or a #5659 merge with identical hatch code), start Sparrow bare, runkeepalive keepawake, and show the log linelistener=up restarted=1 reason=no-health-commandand the next heartbeat carrying the health row. - CI is red at the newest run (39047813715, 16:04Z). The only failure is
tests/claude-task-notifier-inflight.test.py; this PR does not touch it, so it looks like a flake. Re-run CI to green. license/cla is still absent.
#5659: git merge-tree of cebae83 with 2def660 is conflict-free (tree 72641e60).
Item 4: I withdraw my merge condition to file a backend issue. The broker health row is shown on prd. The acp-name-only rule exists only in backend dev's owner rollup, a separate dev question, not a gate here.
…and match; notes name keepawake Two tests pin what keepawake replaces. A Sparrow a systemd unit runs (the one install wrote, or a hand-made one) is never replaced, even when its REMOTE_HEALTH_COMMAND is missing. Without a unit, the running Sparrow's REMOTE_HEALTH_COMMAND must equal the one this install would start it with: a missing, empty, prefixed, truncated or different value means replace, an equal one means leave it, and an unreadable environment means leave it alone. Mutation-checked: dropping the unit guard, or loosening the match to a truthiness, prefix or suffix test, or replacing on an unreadable environment, each fails a test. The install, verify and uninstall notes, the --no-service and --no-start help, and the README no longer say to keep Sparrow running another way: the agent keeps it running with `<reply_command> keepalive keepawake` from its scheduler. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@cla-assistant check |
|
Live witness of the new keepawake path, as requested, on a Hatch agent at build Steps run by the agent today, about 10:16 PDT. The agent reported these lines; it didn't paste the full output.
Still owed: the heartbeat carrying the health row. I've asked the agent for that output and will add it here. CI on |
|
Addendum to the witness above: the health row. A read-only re-check on the same agent at |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Review from sutando-qingyun-001 (shared qingyun-wu login)
APPROVE at a0050e6. Every ask from my cebae83 REQUEST_CHANGES is met.
- Delta (1 commit): the code changes only the notes.
keepawake_note()now names<reply> keepalive keepawakein install,--no-service, verify and uninstall, and says "nothing restarts Sparrow" when there is no keepalive. The rest is docs, help and tests. No behaviour change. - Tests: test_hatch_adapter gives 170 passed, 2 skipped. The wheel test passes 4/4.
- Mutations:
- Dropping the
_unit_owner()guard fails the unit-guard test. - A substring match fails the exact-match test.
- Accepting any non-empty value fails the exact-match test and the different-command test.
- Dropping the
- Witness: it ran at 6e87ac5, a0050e6 merged with #5659 2def660. Its hatch diff from a0050e6 is #5659's own, line for line, and keepalive.py is identical. On a bare Sparrow it showed
listener=up restarted=1 reason=no-health-command, then verify PASS and a heartbeat carryinghealth=abnormal (task-not-picked-up), which is the sequence asked. The lines are as the agent reported them; no full output was pasted. - CI, newest run per check: 33 pass, 3 skipped, none red.
- Merge gate, not a code issue:
license/clais absent on a0050e6. Close and reopen the PR before merging.
|
@cla-assistant check |
What changed, and why
Evidence from a live Hatch agent changed the design. Every probe line said
listener=up, with zero restarts, so the listener was never the weak point. What failed was the agent going quiet: Hatch's scheduler cancelled every run of the 5- and 15-minute jobs withscheduled_occurrence_coalesced, while a daily job kept running, and a VM reboot did not help. A probe inside that same scheduler cannot detect this, and two jobs only double the exposure.So this PR now ships one scheduled task plus health reporting:
sparrow-keepawake(every 15 min) replaces bothsparrow-watchdogandkeepalive-probe. The agent registers it through its own scheduler; install prints the exact command on aREQUIRED keepalive:line (see decision 1). It runstimeout 120 ag2-sparrow-hatch keepalive keepawake. The run itself is what keeps the agent awake. It restarts Sparrow if Sparrow isn't running, using the watchdog's logic: through the systemd unit when install wrote one (reset-failed, thenrestart), else a detached spawn. Exit 75 means another Sparrow holds the lock and is not counted as a restart. Each run appends one line:<utc> keepawake listener=up|down restarted=0|1.health, so AG2 Space's health panel shows the core as "limited" ("Update AG2 Space for health status"). Sparrow gains a generic seam:REMOTE_HEALTH_COMMAND, in the newag2_sparrow/health_command.py. Sparrow runs the injected command at most once a minute and sends the JSON row it prints as the heartbeat's corehealth, with theworker_health.v1capability. The Hatch adapter injectsag2-sparrow-hatch keepalive health. The row ishealthywhile the last keepawake line is at most 30 minutes old (2 intervals); otherwise it isabnormalwith reasonkeepawake-not-running. A task that has never run readsunknownfor 30 minutes from the time install wrote it, thenabnormal, so a scheduler that never runs it shows up 30 minutes after install.sparrow-watchdogandkeepalive-probetask files an earlier install wrote (recognized by the marker comment). It keeps hand-made ones unless--forceis passed.--no-keepaliveanduninstall --keepalivestay./proc/<pid>/environlacksREMOTE_HEALTH_COMMANDis stopped (SIGTERM, then up to 20 s for it to exit; never SIGKILL) and started through the keepawake spawn path, which sets it. One whose environment cannot be read (no/proc) is left running with aNOTE sparrow:line. If none runs, one is started.--no-startskips the step.NOTE sparrow:line, and nothing is started.ExecMainStartTimestampMonotonic).keepalive statusprints listener, last keepawake run and health, and writes nothing.keepalive reportreads the new lines and the old probe and bare-timestamp lines in one report.Setup steps move to AG2 Space (94f5f1c)
The ordered setup steps and their wording now belong to AG2 Space's
agent.setup.stepsAction. Install prints only what the host computes, as data lines an agent orverifycan grep:REQUIRED keepalive: sparrow-keepawake every 15 minutes: timeout 120 <abs>/ag2-sparrow-hatch keepalive keepawake (register it through your scheduler)REQUIRED hook: <staged definition path> (register it through your hooks tool)on a first install, or(update hook ag2-sparrow-hatch to it through your hooks tool)when a registered hook changes, followed in both cases by the definition JSON with its versionedscript_path.keepalive: <task file> (install record), log <log path>Removed: install's trailing
Next: ...paragraph, the multi-sentence hooks-tool how-to on theREQUIRED hook:line, and the README's paste-to-agent text, which is now a short pointer toagent.setup.steps, theninstallandverify. The files table, verify checks, health reasons and upgrade notes stay. Logic, files written, verify, health and the version (0.5.4.dev1) are unchanged.Install output on Muse's case (a registered hook whose script changed; scratch
HOME, paths shortened to~, prompt elided):Lone-mention fix (47f3d74, reverted at 364f636): superseded by the separate PR that removes --answering.
Live finding: re-running install stopped the Hatch hook (fixed at 2e73344 and 368f657)
Root cause (second witness, at 2e73344):
hooks.view ag2-sparrow-hatchfailed withdefinition missing version field, andhooks.listput the hook in its invalid list. The definition install wrote had noversion, so when an earlier install rewrote~/hooks/definitions/ag2-sparrow-hatch.jsonat 19:38, Hatch dropped the hook as invalid. Re-adding it withhooks.add, thenhooks.dry_runandhooks.enable, brought it back. A definitionhooks.addwrites holdsversion(the number 1),id,script_path(a literal~/...),poll_interval_secs,script_timeout_secsandprompt, plus fields Hatch owns:enabled,delivery(an agent-specific target),presentation_localeand timestamps. So at 368f657:"version": 1.installnever writes~/hooks/definitions/, not even on a first install, because it cannot supply the fields Hatch owns. It stages the definition in~/.config/ag2-sparrow-hatch/hook-definition.jsonand prints it after everyREQUIRED hook:line, forhooks.addorhooks.update.verifyfails a registered definition that lacks any ofid,version,poll_interval_secs,prompt,script_path,script_timeout_secs(... lacks version: Hatch marks this hook invalid and never runs it), and one that is missing (the hook is not registered). It compares only the fields install sets.0.5.4.dev1. The second witness ran the old code:pip install -Ufrom git kept the installed 0.5.2 because the version had not changed. The README now says to pass--force-reinstallwhen upgrading from git with an unchanged version.First witness (at 03df3a2):
On a live Hatch agent,
ag2-sparrow-hatch install(at 03df3a2) rewrote~/hooks/scripts/ag2-sparrow-hatch.sh(same content: its sha matchedinstalled-script.sha256) and~/.config/ag2-sparrow-hatch/config. The hook (~/hooks/definitions/ag2-sparrow-hatch.json, a 5 s poll) ran one more poll 24 seconds later and never again. Sparrow kept writing task files: running the hook's task reader by hand listed all 16 open tasks, none inseen_ids. This matches the scheduled-task finding in decision 1: Hatch runs what its own tool registered, and files changed behind that tool stop being run. No public Hatch documentation describes hook reloading, so the fix assumes the observed behaviour and never touches a defined hook's files.Install never rewrites a defined hook. Once
~/hooks/definitions/ag2-sparrow-hatch.jsonexists:chmod(unchanged: hook ag2-sparrow-hatch (...; install did not touch them));~/hooks/scripts/ag2-sparrow-hatch.<sha12>.sh, beside the running one, which is left as it is. The new definition, naming that script, goes to~/.config/ag2-sparrow-hatch/hook-definition.json. Install prints aREQUIRED hook:line and the exact definition, and the agent updates the hook through its hooks tool. A locally modified running script is still shown as a diff and refused without--force.REQUIRED hook:line (368f657; shortened to data at 94f5f1c, see above).Config and the keepawake task file are written only when their content changes, to a temporary file renamed into place, with no
chmodwhen the mode is already right.The hook stamps each run. After taking its lock, every run except
hooks.dry_runrewritesSTATE_DIR/hatch-hook-last-runwith<epoch> <sha256 of the running script>.verify has a new
hook runcheck. It fails when the hook has never run, when its last run is over 2 minutes old (hook not running: last run <ts> (<n>s ago)), or when the stamp's sha is not this ag2-sparrow's script, so it fails until the updated hook has actually run.hook scriptandhook definitionread the script the registered definition names, and fail while the registered definition is not the staged one.The health row gains two reasons that a fresh keepawake line no longer masks:
hook-not-running: the stamp is over 5 minutes old (a hook that never ran is timed from install's last script write);task-not-picked-up: the oldest open task (list_open_taskswith no instance filter, the agent's own messages excepted) is over 15 minutes old.Precedence:
keepawake-not-running, thenhook-not-running, thentask-not-picked-up;sinceis when the named one went bad. The slugs fit the broker's^[a-z0-9-]{1,40}$, and each row passeshealth_command.validate. A task handed off to the main session stays open, so it also readstask-not-picked-upuntil answered.What an existing Hatch agent does to upgrade:
pip install -U "ag2-sparrow[hatch]"(from git: add--force-reinstall; 0.5.4.dev1 is above 0.5.2 and 0.5.3, so-Ualone now upgrades), thenag2-sparrow-hatch install --mxid <its MXID>with the same flags as before. Its running hook and the hook's files are not touched.hooks.addwithversion: 1), install printsinstalled: ~/hooks/scripts/ag2-sparrow-hatch.899573e4cb74.sh (the running hook's ~/hooks/scripts/ag2-sparrow-hatch.sh was left as it is), thenstaged: ~/.config/ag2-sparrow-hatch/hook-definition.json, thenREQUIRED hook: ~/.config/ag2-sparrow-hatch/hook-definition.json (update hook ag2-sparrow-hatch to it through your hooks tool), then the definition JSON.agent.setup.steps, then confirm the hook is listed and enabled.ag2-sparrow-hatch verify:PASS hook run: last run <n>s ago, this ag2-sparrow's scriptonce the updated hook has polled. An agent whose hook already stopped needs only steps 2 and 3, since nothing it does by hand to the files restarts it.Evidence, scratch
HOME(--no-service --no-start --no-keepalive; the running hook is the 25a44c7 script, the same as at 03df3a2):Mutation checks at 2e73344. Each mutation was applied alone, the named test was run, and the file was restored:
chmod-edtest_install_twice_touches_no_hook_or_config_file(mtime, ctime, inode)test_install_upgrades_a_copy_it_wrotetest_install_upgrades_a_copy_it_wrotetest_a_changed_config_is_rewritten_atomicallytest_every_run_stamps_when_and_which_scripttest_dry_run_writes_nothingtest_a_stale_hook_run_is_abnormal_and_fails_verifytest_install_upgrades_a_copy_it_wrotetest_a_hook_that_never_ran_ages_from_installtest_a_stale_hook_run_is_abnormal_and_fails_verifytest_a_hook_that_never_ran_ages_from_installtest_an_old_open_task_is_abnormal_despite_a_fresh_keepawaketest_an_old_open_task_is_abnormal_despite_a_fresh_keepawaketest_an_old_open_task_is_abnormal_despite_a_fresh_keepawakeversiontest_the_definition_has_every_field_hatch_requires(staged and printed)test_verify_fails_a_definition_hatch_marks_invalid~/hooks/definitionstest_install_writes_script_definition_and_configThe health seam (and what the broker expects)
The seam lives in
ag2_sparrow/health_command.py, which names no adapter, asdocs/architecture-boundaries.md"Optional adapter capabilities" requires. The adapter injects the command through Sparrow's environment:Environment="REMOTE_HEALTH_COMMAND=<ag2-sparrow-hatch> keepalive health";The bridge changes by one line in
_post_heartbeat:health = _HEALTH_COMMAND.row() if _HEALTH_COMMAND else _core_health(_health_snapshot()).How the seam runs the command:
shlexsplit, with no shell.HealthCommand.row()returns the last cached row at once and starts a due refresh on a daemon thread, with at most one in flight (the same pattern as_push_pool_advertisement). The first heartbeat after start has no row yet._on_task_env).HEARTBEAT_INTERVAL(60 s), inside the broker's 120 s freshness window (agent_health.FRESH).Field contract. The row is validated against the broker's contract before it is sent. On the
devbranch of ag2-space/ag2space-backend:broker/health_rows.pyvalidate_health, whichapi/v1.pypost_heartbeatcalls onpayload["health"]and answers with a 400 for the whole heartbeat when it fails, requires:alive: bool or nullmotion∈idle|moving|unknowncondition∈healthy|abnormal|unknownreason: a slug matching^[a-z0-9-]{1,40}$, or nullsince: epoch seconds or null (display-only; dropped on receipt)broker/agent_health.pyreportmarks the agentdetail: "full"only whenhealthis an object andcapabilitiescontainsworker_health.v1.health_rows._fullthen showscondition/reason.keepawake-not-runningis not inSELF_HEALING, so it reads asneeds_attention.A row that fails validation is dropped and logged once rather than sent, so the injected command can never get the heartbeat refused. That guarantee covers only the command path; the monorepo snapshot row still goes through
_health_row's normalization, notvalidate.One owner for the contract.
health_command.pydefinesFIELDS, the reason grammar (REASON_RE,REASON_MAX) andslug(). The bridge'sHEALTH_FIELDSishealth_command.FIELDS, and_health_rowcallshealth_command.slug. A test intests/gateway-health-push.test.pypins both paths to that single definition.Presence is unchanged. The injected row feeds the health panel only. The bridge's
_reported_core_statusgetsNonefor it, so a Hatch agent still sends no presencestatus, as before this PR. Without that, the broker mapsstatus: idleto "available" (docs/presence-layer-design.mdin the backend), whateverinflightsays, so a healthy agent would have read available mid-task.What the Hatch row contains:
keepawakelineconditionreasonalivemotionhealthytrueunknownabnormalkeepawake-not-runningtrueif the activity marker (ACTIVITY_FILE, default~/.muse/last_active) changed in the last 30 min, else nullunknownunknownfor 30 min, thenabnormal/keepawake-not-runningunknownsparrow-keepawaketask installedDesign decisions (please check)
~/workspace/cron.d/minutely/. The live witness at 03df3a2 showed it does not. The file install wrote was never listed in the agent's task list and never ran in about 55 minutes. Once the agent created the same task through its scheduler tool, it was listed at once (every 15 min, idsparrow-keepawake). The health row readabnormal/keepawake-not-runningandverifysaid so throughout, which is kept.REQUIRED keepalive: sparrow-keepawake every 15 minutes: timeout 120 <abs>/ag2-sparrow-hatch keepalive keepawake (register it through your scheduler)(data only since 94f5f1c; the step wording is AG2 Space'sagent.setup.steps).verifyprints that step on aNOTE keepalive:line while the row isunknown, and on a countedFAIL keepalive:line once it isabnormal.sparrow-keepawake. It is generic, not tied to any one agent, and it is a new name, so it never collides with the old pair. The.mdbody is unchanged in shape: a marker comment, then "run this one command". The hard timeout is thetimeout 120wrapper in the task's command.verifyalready requirestimeoutonPATH. Two systemd calls of up to 30 s each, plus a 3 s spawn settle, fit inside it. A run killed by the timeout writes no line, so it shows up as a gap and, after 30 minutes, askeepawake-not-running.~/keepalive-probe.log. The new lines land in the same file the old probe wrote, soreportreads a migrated agent's history in one report. The name now says "probe" for a log the keepawake writes; renaming it would split the history.aliveon a stale row is null (unknown), not false. The hook can still wake the agent, so a stopped scheduler is not proof the agent is dead.aliveis true when the hook's activity marker is fresh, or after a keepawake run in the last 30 minutes. The row does not drive the presencestatus; see "Presence is unchanged" above.motionis alwaysunknown. The hook's heartbeat wake touches the activity marker every 5 minutes, so the marker cannot tell working from idle.sparrow-keepawaketask file's mtime._install_fileleaves an unchanged file's mtime alone, so that is when install wrote it. A fresh install readsunknownfor 30 minutes: neither red nor a green with no run behind it (owner's decision). A task the scheduler never runs goesabnormalafter that. Without the task (--no-keepalive,uninstall --keepalive) there is no row; with--no-keepalivethe unit carries noREMOTE_HEALTH_COMMAND, so nothing is spawned.shlex.joinand parsed withshlex.split, so the spawn path survives a path with a space.render_servicewritesEnvironment="K=V"only for a value containing a space; every other row is byte-identical to before. Each word of the command still passes_unit_value, which refuses a path with a space for the unit and prints a NOTE.0 <= now - ts <= STALE_S, for the keepawake line, the install time and the activity marker. A timestamp from the future, such as after a wall clock stepped back, reads stale, not healthy.keepalive watchdog/keepalive probe. Install removes the tasks that ran them. A task file left behind (hand-made, kept without--force) fails with an argparse error rather than silently running the old behaviour.Files to look at first
packages/ag2-sparrow/ag2_sparrow/health_command.py(new): the runner and the broker-contract validation.packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py: the seam's construction and the one-line change in_post_heartbeat.packages/ag2-sparrow/ag2_sparrow/adapters/hatch/keepalive.py: keepawake,health_row, task install/migration, status, report.packages/ag2-sparrow/ag2_sparrow/adapters/hatch/cli.py:install_hook(never rewrites a defined hook; stages a change), thehook runverify check, the unit'sREMOTE_HEALTH_COMMAND, thekeepaliveactions.packages/ag2-sparrow/ag2_sparrow/adapters/hatch/ag2-sparrow-hatch.sh: the per-runhatch-hook-last-runstamp.packages/ag2-sparrow/README.md: the Keepalive section (one job, the health panel instead of the probe), a generic "Reporting your agent's health" section, a pointer to AG2 Space'sagent.setup.stepsin place of the paste-to-agent text, the files table and the example unit.User behaviour this proves
A Hatch agent that runs
ag2-sparrow-hatch installis left with Sparrow running and connected, with the health command in its environment, and with the exact command for its one scheduled task. Its old watchdog and probe tasks are cleaned up. AG2 Space's health panel shows its core as healthy, or as needing attention withkeepawake-not-runningonce the scheduler stops running the task, including when it never runs it at all. Before, it only showed the "update" placeholder.Feature/documentation consistency
The canonical doc for the Hatch adapter and for Sparrow's environment variables is
packages/ag2-sparrow/README.md, updated here.docs/catalog.jsonhas no entry for the Hatch adapter, and its Sparrow docs (the delivery contract and identity) are not touched by this PR.Before/after evidence
Both runs use the same fresh
HOMEand a stubag2-sparrowthat exits 75; hook and config lines are trimmed. Theafterrun is the upgrade path: it starts from what the old install left.A task the scheduler never runs (fresh
HOME, install, then the task file aged 6 hours with no keepawake line):Install starts Sparrow and names the scheduler step; verify fails a task that never ran (fresh
HOME, no systemd, a stubag2-sparrowthat writes a connected status file;verify's other lines trimmed):The no-systemd replace path needs
/proc, so it is not shown from this Mac.test_a_sparrow_without_the_health_command_is_replacedcovers it, and CI on Linux runstest_sparrow_env_reads_a_sparrow_process_onlyagainst a real/proc.The heartbeat body of a standalone Sparrow, built outside the monorepo so there is no
src/health_snapshot.py, withREMOTE_HEALTH_COMMANDset._reqis mocked; this is what_post_heartbeatwould POST to/v1/heartbeat:Tests
python3 packages/ag2-sparrow/tests/test_hatch_adapter.py, as CI runs it, on python3.14: 111 tests at a93ded1 and 153 at 368f657 (2 skipped here: they need/proc), all passing.Hook tests (2e73344): install twice leaves the script, definition, config and marker byte-for-byte untouched (mtime, ctime, inode); a changed config is a new inode at mode 600; an upgrade leaves the running hook's files alone, stages a versioned script and the definition, prints both, and verify fails (
hook script,hook definition,hook run) until the hooks tool takes the definition and the new script has run; every hook run stamps time and sha, and a dry run does not; a stale stamp makes verify FAIL and the rowhook-not-runningwith a fresh keepawake line; a hook that never ran ages from install; a 20-minute-old open task (any instance) with a fresh keepawake line and a fresh hook stamp istask-not-picked-up, while the agent's own, answered and fresh tasks are not.Keepaliveclass (19 tests):unknownright after install,abnormal/keepawake-not-runningonce its file is older than 30 minutes; old probe lines do not count as a run; no task means no row even with a fresh linetimeout 120, and idempotent on re-installsparrow-watchdog/keepalive-probetasks removed, and install's own copy at another interval replaced--force--no-keepalive, anduninstall --keepalive(also removes old-name tasks, keeps the log, then no health row)--cron-dirand--keepalive-logstatuswrites nothingreset-failedthenrestart,restarted=1)REMOTE_HEALTH_COMMANDalivefrom the activity marker, none without a keepawake line; each row passes the broker-contract validatorREMOTE_HEALTH_COMMAND, and not with--no-keepalivereportover old and new linesverifynoteStartSparrowclass (8 tests):-m ag2_sparrow), not a path that merely containsag2-sparrowREMOTE_HEALTH_COMMAND, waits for it to connect--no-startskips the stepsparrow_envreads a real Sparrow process's environment and nothing else (Linux,/proc)HealthCommandclass (11 tests):TemporaryFileraisingOSError(28)) clears the cached row, logs once and starts no further threads within the TTLrow()returns at once while a slow command runs, then serves the refreshed row, and runs nothing again within the TTLalive, a non-zero exit, a timeout and a missing commandag2-sparrow-hatch keepalive health, withworker_health.v1statusREMOTE_TASK_TOKEN: the test fails when the environment filter is removed (checked by hand)tests/gateway-health-push.test.py(the monorepo snapshot path, 17 tests) passes, including the newOneContracttests: the bridge'sHEALTH_FIELDSishealth_command.FIELDS,_health_rowcallshealth_command.slug, and a snapshot row passesvalidate.Mutation checks at 9622ee0: skipping the replace, skipping the start, skipping the connected check, not stopping the outside Sparrow, and reading the first 4 KB of stderr each fail their test. At 03df3a2: removing the
except Exceptioninrefreshfails the raise test; removing the single-flight guard fails the one-at-a-time test; readinghealthybefore any run fails the never-run, status and verify tests. At 5d25649, each caught by the named test: dropping0 <=(clock-skew test);TIMEOUT_S = 3600(timeout test); refreshing inline inrow()(the never-waits test and the heartbeat tests); keeping the old row on error (error-clears test); reading the first keepawake line instead of the last (last-line test); removing the output bound (bound test). Earlier, at a9c8dce: makinghealth_rowreturn None when there is no keepawake line fails the never-run, old-probe-lines, status and verify tests. Restoring the bridge's own field tuple and slug regex fails theOneContractpin. Passing the injected row to_reported_core_statusfails the heartbeat test.Also as does
ruff check packages/ag2-sparrow.At 94f5f1c: 165 tests, all passing (2 skipped,
/proc). The changed tests assert on the data lines. Mutation checks, each caught: rewording the first-installREQUIRED hook:clause (test_install_writes_script_definition_and_config); rewording the update clause (test_install_upgrades_a_copy_it_wrote,test_verify_fails_a_definition_hatch_marks_invalid); printing aNext:line again (test_install_schedules_one_task_under_a_hard_timeout,test_linger_failure_is_not_fatal); changing theREQUIRED keepalive:line or the install-record line (test_install_schedules_one_task_under_a_hard_timeout); printing the definition before itsREQUIRED hook:line (test_the_definition_has_every_field_hatch_requires,test_install_writes_script_definition_and_config); dropping the definition after the update line (test_install_upgrades_a_copy_it_wrote).Live finding: a bare-relaunched Sparrow drops the health row (c62427e, cebae83)
On a Hatch VM with no systemd, Sparrow died and was relaunched bare (
launched_via=bare) by something other than the keepawake task, withoutREMOTE_HEALTH_COMMANDin its environment. Its heartbeats stopped carrying the health row, the health panel lost the agent's status, and keepawake kept logginglistener=up restarted=0. Re-running install fixed it, since install already replaced such a Sparrow.keepawake: with no unit install found, a running Sparrow whose/proc/<pid>/environlacks this install'sREMOTE_HEALTH_COMMANDis replaced throughreplace_sparrow, the helper install now uses too, and the line readslistener=up restarted=1 reason=no-health-command. An unreadable environment is left running withreason=unknown-env.verify:FAIL sparrow health: sparrow runs without the health command (pid N): its heartbeats carry no health row; run install;NOTE sparrow health:when the environment cannot be read.keepalive statusends withflag=sparrow-no-health-command. The health row does not carry it: a Sparrow without the command never runs the command that prints the row.barecomes from:remote_gateway_bridge._LAUNCHED_VIAisbarewheneverSUTANDO_SUPERVISEDis unset, which is every Hatch launch path (the user unit, install's and keepawake's spawn, a hand-runag2-sparrow), sobarealone does not name the launcher.sparrowcheck's hint says, and that hint said "start ag2-sparrow again as at install". A worker doing that starts Sparrow withoutREMOTE_HEALTH_COMMAND.cli.restart_hint(), used by the hook's wake,verifyandag2-sparrow-hatch sparrow) is nowrestart it: run <reply_command> keepalive keepawake, which restarts the installed service or starts Sparrow with the health command. The hook's own wake text ("Restart Sparrow as that says") does not restate it and is unchanged.Tests: 169 in
test_hatch_adapter.py, all passing (2 skipped,/proc). Mutation checks, each caught: keepawake not replacing (test_keepawake_replaces_a_sparrow_without_the_health_command); verify not failing, or not noting an unreadable environment, or status not flagging (test_verify_fails_a_sparrow_without_the_health_command); an unreadable environment not logged asunknown-env(test_keepawake_leaves_a_sparrow_it_cannot_read_or_that_has_it); the hint back to "start ag2-sparrow again as at install", or adding a bareag2-sparrowto it (test_the_restart_hint_runs_keepawake_never_a_bare_sparrow).🤖 Generated with Claude Code