Repository navigation
feat(sparrow-hatch): install sets up the ag2-sparrow systemd user service so Sparrow restarts by itself - #5488
Conversation
…vice Where systemctl --user reaches a user manager, ag2-sparrow-hatch install writes ~/.config/systemd/user/ag2-sparrow.service (the README unit, with the task/result/state dirs and GATEWAY_INSTANCE install resolved, token read via AG2_DEVICE_ENV, never inlined), runs daemon-reload when it changed, enable --now, and loginctl enable-linger. Every failure is a NOTE, never a failed install. A Sparrow already running outside systemd is left alone (enabled, not started) with switch-over steps; a missing token file enables without starting. RestartPreventExitStatus=75 stops a restart loop on the single-instance lock. --no-service opts out; verify reports the service. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 91%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
packages/ag2-sparrow/ag2_sparrow/adapters/hatch/cli.pyLines 219-228 219 Never prompts: over SSH a polkit password prompt would hang until the timeout."""
220 try:
221 return subprocess.run([cmd[0], "--no-ask-password", *cmd[1:]], stdin=subprocess.DEVNULL,
222 capture_output=True, text=True, timeout=SYSTEMD_TIMEOUT_S)
! 223 except (OSError, subprocess.SubprocessError):
! 224 return None
225
226
227 def _ok(r) -> bool:
228 return r is not None and r.returncode == 0Lines 268-276 268 def _unit_props(*names: str) -> dict:
269 r = _systemd_run(["systemctl", "--user", "show", SERVICE_NAME,
270 *(f"--property={n}" for n in names)])
271 if not _ok(r):
! 272 return {}
273 return dict(line.split("=", 1) for line in r.stdout.splitlines() if "=" in line)
274
275
276 def _outside_sparrow(state_dir: Path, instance: str, main_pid: int, ran: bool) -> bool:Lines 283-297 283 holder = None
284 if isinstance(holder, dict) and holder.get("host") == workspace_lock._host_label():
285 pid = holder.get("pid")
286 if not isinstance(pid, int) or isinstance(pid, bool) or pid <= 0 or pid == main_pid:
! 287 return False
288 try:
289 os.kill(pid, 0)
290 except ProcessLookupError:
291 return False # its holder crashed; the fresh status file is a corpse
! 292 except OSError:
! 293 pass
294 return workspace_lock._is_fresh(holder, workspace_lock.DEFAULT_STALE_SECONDS)
295 # No lock to read: a fresh status file is another Sparrow's only if this unit never ran.
296 code, _ = check_sparrow(state_dir, instance=instance)
297 return code != SPARROW_DOWN and not ranLines 309-319 309 "restarts Sparrow after a crash or reboot; keep ag2-sparrow running another way")
310 return
311 exec_start = _find_console_script(SPARROW_COMMAND)
312 if not exec_start:
! 313 note(f"{SPARROW_COMMAND} was not found beside {sys.executable} or on PATH; no service "
314 "was set up. Reinstall ag2-sparrow for this Python and run install again")
! 315 return
316 device_env = device_env_path()
317 try:
318 st = device_env.stat()
319 except OSError:Lines 359-368 359 state = props.get("ActiveState", "inactive")
360 active = state in ("active", "reloading")
361 try:
362 main_pid = int(props.get("MainPID") or 0)
! 363 except ValueError:
! 364 main_pid = 0
365 ran = props.get("ExecMainStartTimestampMonotonic", "0") not in ("", "0")
366 # failed/activating/deactivating: the unit's own Sparrow crashed or is restarting.
367 outside = state == "inactive" and _outside_sparrow(state_dir, instance, main_pid, ran)
368 has_token = device_env.is_file()Lines 371-381 371 _systemd_run(["systemctl", "--user", "reset-failed", SERVICE_NAME])
372 r = _systemd_run(["systemctl", "--user", "enable", *(["--now"] if start else []),
373 SERVICE_NAME])
374 if not _ok(r):
! 375 note(f"systemctl --user enable {SERVICE_NAME} failed ({_why(r)}); fix that and run "
376 f"systemctl --user enable --now {SERVICE_NAME}")
! 377 return
378 if active and changed:
379 r = _systemd_run(["systemctl", "--user", "restart", SERVICE_NAME])
380 print(f"restarted: {SERVICE_NAME} with the new unit" if _ok(r) else
381 f"NOTE service: restart failed ({_why(r)}); run systemctl --user restart "Lines 623-633 623 return 1
624 if systemd_user_available():
625 r = _systemd_run(["systemctl", "--user", "disable", "--now", SERVICE_NAME])
626 if not _ok(r):
! 627 print(f"error: systemctl --user disable --now {SERVICE_NAME} failed ({_why(r)}); "
628 f"{unit} was kept", file=sys.stderr)
! 629 return 1
630 print(f"stopped and disabled: {SERVICE_NAME}")
631 unit.unlink()
632 print(f"removed: {unit}")
633 if systemd_user_available(): |
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed at 67437d8 (fresh clone, python3.14, review-preflight read, scripts/review-checks.sh --diff on the PR diff).
Verified
- Token never in the unit.
render_servicewrites onlyEnvironment=rows forAG2_DEVICE_ENV(a path) plus the threeAGENT_CONNECT_*dirs and optionalGATEWAY_INSTANCE;ExecStarthas no args. Sparrow reads the token from theAG2_DEVICE_ENVfile (remote_gateway_bridge.py:517,_channel_env_candidates).test_install_writes_enables_and_starts_the_user_serviceasserts neithers3cretnorREMOTE_TASK_TOKENis in the unit even with the token exported in the env. Default path~/.config/ag2-sparrow/relay.envmatches the existing onboarding text (README:246 on the parent). - Single-instance rule kept. Sparrow exits 75 when
_acquire_singleton()fails (remote_gateway_bridge.py:6278); the unit setsRestartPreventExitStatus=75, so a second copy is not relaunched. A Sparrow already running outside systemd is left alone (enable without--now, no kill, NOTE with switch-over steps). - Idempotence. Unchanged unit: no write, no
daemon-reload, no restart,enable --nowonly. Marker-less unit: diff + kept unless--force. - Tests. Head:
Ran 86 tests ... OK. Parentcli.pywith the head test file:FAILED (errors=83)—Basepatches_systemd_run, which the parent lacks, so this confirms the dependency rather than the per-test before list; the body's adapted-copy evidence covers that. - Mutations (Service class), each killed by a test:
start = has_token(drop the outside-systemd guard) ->test_a_sparrow_already_running_outside_systemd_is_left_alone; dropRestartPreventExitStatus=75->test_install_writes_...; serviceif old == text:->if False:->test_rerun_is_idempotent; drop theloginctlcall -> 2 failures;--no-serviceignored ->test_no_service_flag. - CI: 33 pass, 3 skipping, 0 fail at this head.
- Hardcoded paths:
review-checks: PASS. The only/home/...additions are README placeholders (/home/you/...). - No
WorkingDirectory: fine — Sparrow does not read cwd (nogetcwd/Path.cwdinag2_sparrow/).
Blocker
loginctl enable-lingercan raise an interactive polkit password prompt._systemd_runusessubprocess.run(cmd, capture_output=True, ...)with stdin inherited. loginctl opens a polkit tty agent when stdin is a TTY and--no-ask-passwordis not given.org.freedesktop.login1.set-self-lingerisyesonly for an active local session; for SSH or inactive sessions the default isauth_admin_keep. A person runninginstallover SSH on a Linux box (the Hatch case) can therefore get a password prompt that the 30 s timeout kills mid-typing. That is fail-soft, but it is not "no prompt". Fix: pass--no-ask-passwordtologinctl(and thesystemctlcalls), and/orstdin=subprocess.DEVNULLin_systemd_run, plus one test that asserts the flag. I have not run this on a Linux host; the claim comes from systemd's polkit-agent behavior (agent only whenisatty(STDIN_FILENO)) and the stock login1 policy.
Non-blocking
- No start limit. With
RestartSec=10, the defaultStartLimitIntervalSec=10s/Burst=5never trips. A missing or bad token at boot (Sparrowsys.exit("FATAL ...")= exit 1) restarts every 10 s forever. ConsiderStartLimitIntervalSec=andStartLimitBurst=in[Unit](for example 300/5), or say in the README that the loop is intended. - Env-file permissions are not checked. Install never creates or chmods
relay.env. The missing-file NOTE sayschmod 600, but an existing world-readable file passes silently. AverifyNOTE when mode & 0o077 != 0 or the owner is not the current user would close this. - No removal path. There is no
uninstall(that is pre-existing for the hook too), and the README does not say how to remove the service. One line would do:systemctl --user disable --now ag2-sparrow.service && rm ~/.config/systemd/user/ag2-sparrow.service && systemctl --user daemon-reload.
Overlap with #5460
A trial merge of #5460 (f218d78) onto this head conflicts in one hunk only: the constants block in cli.py (this PR's SPARROW_COMMAND/SERVICE_*, and #5460's ANSWERING_MIN_WORDS/_MENTION/_WORD). Resolve it by keeping both. README and tests auto-merge. Suggested order: #5460 first, since it is a correctness fix for reply routing, then rebase this PR and rerun tests.test_hatch_adapter plus tests/hatch-reply-real-drain.test.py.
Verdict: REQUEST_CHANGES on item 1 only. It is a small fix, and I will approve once --no-ask-password / stdin=DEVNULL lands with a test.
- A failed, restarting, or lock-holder-dead service is the unit's own crash: install starts it (reset-failed first) instead of calling it an outside Sparrow. Outside = inactive unit and a live non-service holder of Sparrow's single-instance lock; without a lock, a fresh status file and a unit that has not run since boot. - Restart=always: Sparrow's only exit-0 path is SIGTERM/SIGINT (_exit_on_signal); systemctl stop still stops it. RestartPreventExitStatus=75 stays; a test pins that a second Sparrow exits 75 on a held lock. - Unit values: refuse only whitespace, quotes, backslash, $ and ; (non-ASCII allowed, % escaped), with a NOTE naming the character. - Linger: skip when Linger=yes already, and when running as root. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…le check; uninstall --service - Every systemctl/loginctl call passes --no-ask-password and stdin=DEVNULL, so a polkit check over SSH fails at once instead of hanging to the timeout. - [Unit] StartLimitIntervalSec=300 / StartLimitBurst=5: a crash loop that cannot succeed (rejected token) ends failed; install and the restart hint run reset-failed first. - install warns when the AG2_DEVICE_ENV file is group/other-readable or owned by another user. - ag2-sparrow-hatch uninstall --service stops, disables and removes a unit install wrote; README gives the manual steps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@cla-assistant check |
qingyun-wu
left a comment
There was a problem hiding this comment.
Re-reviewed at e50e1f9 (re-fetched; the delta since 67437d8 is 64e1089 + e50e1f9).
My earlier blocker is fixed
_systemd_run now runs [cmd[0], "--no-ask-password", *cmd[1:]] with stdin=subprocess.DEVNULL for every systemctl/loginctl call. test_no_systemd_call_can_prompt_for_a_password pins this: reverting the line to cmd fails that test.
Earlier non-blocking points, now addressed
- Start limit.
StartLimitIntervalSec=300/StartLimitBurst=5in[Unit], withRestart=always.RestartPreventExitStatus=75still applies underalways. - Token file. Install prints a NOTE when the mode has group/other bits (
& 0o077) or the owner uid is not the current user. - Removal.
uninstall --servicedisables with--now, removes the unit and runs daemon-reload. It refuses a unit without the marker. - Exit 75 proven against the real lock.
test_a_second_sparrow_exits_75_which_the_unit_does_not_restarttakesworkspace_lock.acquire("gateway-bridge", home)and then runspython -m ag2_sparrowas a real subprocess, which exits 75. The lock path_outside_sparrowreads,state_dir.parent/state/locks/gateway-bridge<suffix>.lock, matches the bridge's_LOCK_WS = _STATE.parentandworkspace_lock._locks_dir. - Crash vs outside Sparrow. A lock held by a dead pid, or by the unit's own
MainPID, is not treated as an outside Sparrow. Afailedunit getsreset-failedand thenenable --now. - Linger. Skipped when it is already
yesor when running as root.
Evidence
python3.14 -m unittest tests.test_hatch_adapter-> OK.tests/hatch-reply-real-drain.test.py-> OK.- Mutations, each killed by a named test:
- drop
--no-ask-password/DEVNULL ->test_no_systemd_call_can_prompt_for_a_password ProcessLookupError: return False->pass->test_a_just_crashed_service_is_started_not_mistaken_for_an_outside_sparrow(inactive)- linger-already-on check disabled ->
test_linger_already_on_or_root_is_not_enabled_again StartLimitBurst=50->test_install_writes_enables_and_starts_the_user_service- uninstall marker check disabled ->
test_uninstall_service
- drop
- CI: 34 pass, 3 skipping, 0 fail at this head.
scripts/review-checks.sh --diff: PASS. - Not verified: no live run against a real
systemctl --user/loginctlhere (macOS host). Everything above usesFakeSystemd, apart from the real-lock exit-75 test.
#5460
A trial merge still conflicts only in the cli.py constants block, and keeping both sides resolves it. Merge #5460 first, then rebase this PR. After that push the reviews need refreshing: rerun tests.test_hatch_adapter and tests/hatch-reply-real-drain.test.py.
No blockers remain. APPROVE at this head.
liususan091219
left a comment
There was a problem hiding this comment.
Ready to approve.
Reviewed at e50e1f9 (diff read, no local run).
Gate state at this head
- qingyun-wu APPROVED at e50e1f9 with substantive evidence: five mutations, each killed by a named test, and her earlier CHANGES_REQUESTED (the password prompt) is fixed and pinned by
test_no_systemd_call_can_prompt_for_a_password. - CI: all checks pass or skip.
license/clasuccess (1 context). Mergeable, CLEAN. 0 unresolved threads. Basemain.
Change-request conditions checked, none fire
- Tests exercise behaviour. The
read_text()calls intests/test_hatch_adapter.pyread the unit file that install writes, never module source. The exit-75 test runs the realpython -m ag2_sparrowagainst a realworkspace_lock. - No added
~/.sutando,$HOME/.sutandoorSUTANDO_WORKSPACEliterals. - Worst case for existing users (REVIEW.md #5): this is on by default, but anyone who followed the old README already has
ag2-sparrow.servicewithout the marker. That unit is kept and only shown as a diff (install_service, thenot old.startswith(SERVICE_MARKER) and not forcebranch). A Sparrow running outside systemd is enabled but not started, and nothing is killed.
Owed, not blocking: a live round trip on a real systemctl --user/loginctl host. Both reviews so far used FakeSystemd.
Merge order: #5460 (open) conflicts in the cli.py constants block. Land #5460 first, then rebase this PR and refresh the reviews.
Nits (non-blocking)
uninstall(): whensystemctl --useris unavailable, the unit file is deleted withoutdisable, so adefault.target.wantssymlink can be left dangling.uninstall()callssystemd_user_available()twice. One result would do.
Not verified by me: I did not run the suite or the mutations myself. I relied on CI and the approver's report for those.
— Sutando (susans-MacBook-Pro)
Why
The Hatch onboarding says nothing restarts Sparrow by itself, and the README has only a systemd example. A real Meta AI Hatch agent's listener ran as a plain background process and was dead for about 6 hours. During that time no AG2 Space message reached the agent. The heartbeat only noticed and asked the worker to restart it.
What
When
systemctl --userreaches a user manager (checked withsystemctl --user show-environment),ag2-sparrow-hatch installnow does the following:~/.config/systemd/user/ag2-sparrow.serviceis written with:ExecStart: theag2-sparrowbeside install's Python, else the one onPATH.AG2_DEVICE_ENV, which points at the token.envfile. The token value is never written to the unit.AGENT_CONNECT_TASK/RESULT/STATE_DIRthat install resolved for the hook, plusGATEWAY_INSTANCEwhen one is named.Restart=always,RestartSec=10andRestartPreventExitStatus=75.StartLimitIntervalSec=300andStartLimitBurst=5, in[Unit].daemon-reloadonly when the unit changed, thenenable --now. Thenloginctl enable-linger $USER, which is skipped whenshow-useralready reportsLinger=yesand when install runs as root.systemctlandloginctlcall runs with--no-ask-passwordandstdin=DEVNULL. Over SSH, a polkit check therefore fails at once instead of waiting on a password prompt until the 30s timeout kills it.NOTE service:line saying what was not done and how to finish.--forceis passed.failed,activatingordeactivatingis the service's own crash or restart. Install starts it, runningreset-failedfirst when it is failed.inactiveunit is "outside" only if a live process, other than the unit'sMainPID, holds Sparrow's single-instance lock. With no lock to read, it is outside only if the status file is fresh and the unit has not run since boot.$or;is refused with a NOTE naming the character. Non-ASCII is written as is, and%is escaped to%%.--no-serviceskips all of the above.uninstall --servicestops, disables and removes a unit install wrote, refuses a unit the user wrote, and leaves linger alone. The README also gives the manual steps.verifyprintsPASS servicewhen the unit is installed and enabled, andFAIL servicewhen it is installed but not enabled. It prints aNOTE serviceline, which does not affect the exit code, when systemd is unavailable or the unit is not installed.reset-failedthenrestart.Why
Restart=always, and the exit-75 claim_exit_on_signal, which handles SIGTERM/SIGINT (packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:6127-6144, registered at 6164-6166).main()'s loop at :6299 does not return.Restart=on-failure, a straykill <pid>would leave Sparrow down.systemctl --user stopstill stops it underRestart=always.remote_gateway_bridge.py:6275-6278(if not _acquire_singleton(): sys.exit(75)).test_a_second_sparrow_exits_75_which_the_unit_does_not_restartchecks this end to end: it takes the lock with the realworkspace_lock.acquire, runspython -m ag2_sparrow, and asserts exit code 75.StartLimit*belongs in[Unit]and works alongsideRestart=always. A loop that cannot succeed, such as a rejected token, endsfailedafter 5 starts in 5 minutes, instead of restarting every 10s forever.Evidence
Focused hatch suite,
python3.14 -m unittest tests.test_hatch_adapter, run frompackages/ag2-sparrow. The tests never reach the host'ssystemctlorloginctl:Basemocks_systemd_run, and the one test that runs the real_systemd_runmockssubprocess.run.Before (parent commit 88945c2): the current test file run against the parent's
cli.py. It was run from a temporary copy that usescreate=Truemocks and literals, because the parent has no_systemd_run,SERVICE_MARKERorrender_service. Temp paths are shown as$HOMEand long assertion messages are cut at 230 characters. The 73 pre-existing tests pass and all 20 new ones fail:test_a_second_sparrow_exits_75…errors on the parent only at itsrender_servicecheck. Its exit-75 half tests existing Sparrow behaviour.After (this head):
python3.14 tests/hatch-reply-real-drain.test.pyalso passes:Ran 6 tests ... OK.Not yet verified live
No real
systemctl --userround trip has been run. That would mean install on a host with a user manager,kill -9of Sparrow, Sparrow coming back by itself, and the same after a reboot with linger. It needs a Linux host with a systemd user manager, and the owner's rule is not to runsystemctl/loginctlon this Mac. Everything here is proven only against mocks. The first live check will be on the Hatch VM after release.Merge order
#5460 (
fix/hatch-reply-task-binding) touches the same files and merges first. This PR then gets rebased: onlycli.py's constants block conflicts, and both sides are kept. The hatch tests are rerun after the rebase. The AG2 Space web client's Hatch setup copy (MUSE_HATCH_NOTES) changes only after this ships in a released ag2-sparrow.🤖 Generated with Claude Code