Repository navigation
feat(ag2-sparrow): an injected health row reports moving while a message is in flight - #5682
Conversation
…age is in flight An adapter's REMOTE_HEALTH_COMMAND row (the Hatch keepalive row) always sent motion "unknown", so AG2 Space's health dot could never show the agent at work. Sparrow already knows: its in-flight set holds every message it pulled from the relay until that message's result is posted. health_command.with_motion fills an unknown motion from it (moving while the set is non-empty, idle otherwise); a motion the command prints stands, and the workspace snapshot path is untouched. The heartbeat already beats early on a changed row, so the switch reaches the broker on the next loop pass, not after the command's 60 s refresh. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@cla-assistant check |
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 92%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
akag2
left a comment
There was a problem hiding this comment.
Verified the core claim end to end.
Design: health_command.with_motion(row, busy) lives in the package-core row contract (health_command.py), not the Hatch adapter — correct per the stated rationale, since it names no adapter and any REMOTE_HEALTH_COMMAND reporting unknown motion gets the same behavior for free. It's a pure function: returns row unchanged (same object, confirmed via assertIs in the new test) when row is None or motion != "unknown", else a new dict with motion swapped to moving/idle based on bool(inflight) — no mutation of the cached row.
Call site: _post_heartbeat only applies with_motion on the _HEALTH_COMMAND branch (health_command.with_motion(_HEALTH_COMMAND.row(), bool(inflight))); the _core_health(_health_snapshot()) workspace-snapshot branch is untouched, matching the "Sutando core keeps its own motion" claim — confirmed by the new test_the_snapshot_row_keeps_its_own_motion_whatever_is_in_flight, which asserts unknown passes through unchanged even with a non-empty inflight set.
Evidence: the PR's own before/after capture (parent 60bc40a4 vs HEAD) shows the exact heartbeat payload going from motion: "unknown" → moving (1 in flight) → idle (0 in flight) using the real ag2-sparrow-hatch keepalive health command, not a stub. The new subprocess-based test (test_the_hatch_row_is_moving_while_an_item_is_in_flight_and_idle_after) exercises the same real CLI path. test_a_motion_the_command_prints_is_sent_as_printed pins that a command-reported motion (not unknown) is passed through verbatim, which is the regression this function must not introduce.
Timing claim checked: "doesn't wait for the 60s refresh" is consistent with the existing changed = health is not None and health != _last_core_health early-send gate just below the new call — a motion flip changes the row, which already triggers the early-send path pre-existing in this function. Not new behavior, just correctly riding an existing mechanism.
CI: all 32 checks green (31 pass + 1 informational skip for PowerShell/Windows as usual), diff coverage 100% on every changed file, no existing reviews, no CLA issue, mergeStateStatus: BLOCKED is just the required-review gate (not a conflict — mergeable: MERGEABLE). The "Not covered" section is honest about scope (no live relay round-trip, server-side panel display out of scope) and doesn't overstate what was verified.
No concerns.
liususan091219
left a comment
There was a problem hiding this comment.
Reviewed at 8224a78f2. I found nothing that needs changing. This is a comment rather than an approval because the approval bar isn't met yet.
Why akag2's approval does not count toward the bar
- akag2 has
readpermission andauthor_association: NONE, with no merged PRs in sonichi/sutando, so it isn't a trusted expert for ag2-sparrow, and its approval can't satisfy the main ruleset (reviewDecisionis stillREVIEW_REQUIREDwith it in place). - The review reads the code accurately, but its evidence is the PR's own before/after capture plus a description of the tests; it doesn't say it ran anything.
- Recent merged ag2-sparrow PRs were approved mainly by @qingyun-wu (#5675, #5660, #5618, #5603, #5576, #5488, #5449). That is the expert approval this needs. The community route (3 known approvals) isn't met either.
What I checked
- Design:
health_command.with_motion(packages/ag2-sparrow/ag2_sparrow/health_command.py:65-70) is a pure function and names no adapter, so it sits in the right layer. Its only call site is the_HEALTH_COMMANDbranch of_post_heartbeat(remote_gateway_bridge.py:3512-3513); the snapshot branch is unchanged. - Timing claim:
_post_heartbeat(inflight)runs on every poll-loop pass (:7261,:7326), and a motion flip changes the row, so the existing early send fires (changed = health != _last_core_health,:3514). The PR's statement is accurate. - Sibling search: the only adapter that emits
motion: "unknown"isadapters/hatch/keepalive.py:546; the fix is generic, not a band-aid.src/remote-gateway-bridge.pyis a shim over the package module, not a second copy. - Hardcoded paths in added lines: none. CI: required checks green (3 skipped).
license/cla: success (1 context). Unresolved threads: none.MERGEABLE.
What I ran (fresh clone at the head, SUTANDO_WORKSPACE_DIR/SUTANDO_TASKS_DIR unset, no-space venv):
python -m unittest tests.test_hatch_adapter.HealthCommand(inpackages/ag2-sparrow): 15 OK.tests/gateway-health-push.test.py: 18 OK.- Mutations (each reverted): bridge drops
with_motion→HealthCommandfails 1 (the realkeepalive healthsubprocess test);with_motionalwaysidle→ fails 2; a printed motion overwritten → fails 2;with_motionapplied to the snapshot row too →gateway-health-pushfails 1. Every claimed behaviour is pinned, and the tests call the real modules.
Not verified: a live round trip against the relay (owed per the PR body; named, not blocking); the full pytest packages/ag2-sparrow/tests run (the PR reports 439 passed); whether the server-side health panel displays this row for a Hatch agent.
Minor: a handed-off task that is never answered shows moving indefinitely. The README documents it, consistent with the existing task-not-picked-up condition.
Missing for approval: one ag2-sparrow expert approval at this head, e.g. @qingyun-wu.
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
There was a problem hiding this comment.
Approving at 8224a78f as code owner for /packages/.
- One owner, one place.
with_motionlives inhealth_command.py, the module that already owns the row contract, and_post_heartbeatis its only caller.src/remote-gateway-bridge.pyjust loads the package's bridge, so there is no second copy to keep in step. - It fills only what is missing. A row's motion is replaced only when the command printed
unknown, frombool(inflight). A motion the command prints stands, and aNonerow staysNone. The test also checks that the cached row is not changed in place. - The core snapshot path is untouched.
_core_health(...)rows keep their own motion, andtest_the_snapshot_row_keeps_its_own_motion_whatever_is_in_flightpins that. - The dot updates promptly. A moving-to-idle flip changes the row, so the existing
health != _last_core_healthcheck sends an early heartbeat. The dot follows an item starting and finishing, not the next interval. - The end-to-end test runs the real bridge with the real Hatch
keepalive healthcommand, and readsmoving/healthywith an item in flight, thenidle/healthyafter.
CI is green at this head (34 passed, 3 skipped).
What and why
A Hatch agent's health row (
ag2-sparrow-hatch keepalive health, sent throughREMOTE_HEALTH_COMMAND) always carriesmotion: "unknown", so AG2 Space's health dot can never show that agent at work: no breathing dot while it answers, and no solid "idle" dot otherwise.Sparrow already has the state that answers this. Its in-flight set holds every message it pulled from the relay until that message's result is posted (or a
--no-sendcloses it). So the policy goes in the package core, beside the row contract, not in the Hatch adapter:health_command.with_motion(row, busy): a row whosemotionisunknownis sent asmovingwhile an item is in flight,idleotherwise. Amotionthe command prints stands. No row stays no row. The function names no adapter, so any adapter that reportsunknowngets the same behaviour.remote_gateway_bridge._post_heartbeatapplies it to the injected row only. The workspace-snapshot path (a Sutando core) is unchanged; it has its own motion.alive,condition,reasonandsincecome from the command as before.moving.README: the generic
REMOTE_HEALTH_COMMANDsection and the Hatch health section now describe the motion. A task handed off to the main session stays open, so it readsmovinguntil it is answered.Evidence
The same heartbeat with the real
keepalive healthcommand (fresh keepawake line), with one item in flight and then none:Parent
60bc40a4:HEAD:
New tests in
packages/ag2-sparrow/tests/test_hatch_adapter.py(HealthCommand), run with the parent's code and these tests:HEAD:
15 passed, 154 deselected. Also added:test_a_motion_the_command_prints_is_sent_as_printed, and intests/gateway-health-push.test.pytest_the_snapshot_row_keeps_its_own_motion_whatever_is_in_flight.Full runs at HEAD:
pytest packages/ag2-sparrow/tests:439 passed, 2 skippedtests/gateway-health-push.test.py: 18 OKtests/ag2-sparrow-hatch-adapter.test.py: OK (skipped=2)tests/agent-health-report.test.py: OKtests/ag2-sparrow-drift.test.py: PASSbash scripts/review-checks.sh --diff: PASS (hardcoded-paths, root-artifacts and prose-cap clean)ruff checkon the changed files: cleanNote on running locally: the
HealthCommandheartbeat tests splitsys.executablewith shlex. On an interpreter whose path contains a space they fail at the parent too (FileNotFoundErroron the truncated path). I ran them from a venv whose path has no space.Not covered
healthobject the heartbeat already sends, and the evidence above is that exact payload. The bridge's poll loop and_reqare mocked there.🤖 Generated with Claude Code