[mybelovedstar123]

Priority

Level: low
Reason: a refactor flagged in the #5375 review, so the reminder seat and the create-dedupe can't drift on which device is which.

Closes

Fixes #5376

Summary

renotify.seat_key, manager._device_id and events._device_id each spelled out str((req.device or {}).get("id") or ""). The issue names the first two; events.py had a third copy.

schema.device_id(req) is now the one definition, and renotify, manager and events import it. I chose schema.py because renotify can't import from manager, which already imports renotify, and schema is where HumanRequirement lives. Behaviour is unchanged. The vendored packages/ag2-sparrow/ag2_sparrow/hitl/{renotify,manager,schema}.py are updated byte-identical (events.py isn't vendored).

tests/fixtures/old_main/hitl_pre_local/ still has the old copies. Those are frozen snapshots of past code, so I left them alone.

Checklist

• Confirmed no other open PR closes the same issue
• Git author email matches my CLA-signed email
• Single concern per PR
• Confirmed the duplication on upstream/main (the structural check below names all three files)
• Test added (test_the_seat_and_the_create_dedupe_name_a_device_one_way)
• Before/after evidence pasted below
• Every claim in this PR body is verifiable from the diff or the pasted output
• Live path: N/A. This is a pure refactor with identical behaviour.
• Stacked PR: N/A
• No host-specific paths added
• No CI workflow changes
• No workspace/home-path resolution touched

Before / after evidence

The new test against the parent src/hitl (and vendored copies):

ERROR: ... (device=None) AttributeError: module 'hitl.schema' has no attribute 'device_id'   (x5 subtests)
FAIL: test_the_seat_and_the_create_dedupe_name_a_device_one_way
AssertionError: Lists differ: ['events.py', 'manager.py', 'renotify.py'] != []
Ran 26 tests — FAILED (failures=1, errors=5)

This branch: tests/hitl-renotify.test.py → OK.

Test plan

Linux (WSL2, Python 3.14): all 15 tests/hitl-*, tests/ag2-sparrow-hitl-vendored* and tests/core-input-watch-chat-escalation.test.py (which compares seat_key across a requirement and its event) exit 0. python3 packages/ag2-sparrow/tools/sync_from_src.py --check reports in sync. ruff check is clean, and bash scripts/review-checks.sh --diff <(git diff upstream/main...HEAD) PASS.

Author: discord username :: slovertop :: agent-assisted (Claude)

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 92%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/hitl/events.py (100%)
• src/hitl/renotify.py (100%)
• src/hitl/replies.py (100%)
• src/hitl/schema.py (100%)

Summary

• Total: 6 lines
• Missing: 0 lines
• Coverage: 100%

[sonichi]

Approving at b22f9f9.

schema.device_id(req) is the one definition, and renotify, manager and events now import it, so the reminder seat and the create-dedupe can't name a device two ways. Behaviour is unchanged: it's the same str((req.device or {}).get("id") or "") expression. schema.py is the right home given the manager → renotify import. The vendored renotify, manager and schema are byte-identical to src.

Ran in a clean env: tests/hitl-renotify.test.py OK and tests/ag2-sparrow-drift.test.py PASS.

Nit, not for this PR: hitl/projector.py:280 still reads device.get("id") or "" inline. It reads a device dict rather than a requirement, so it doesn't fit device_id(req), but it's the same rule if it ever matters.

(agent-assisted, Blue for Chi's seat)

[liususan091219]

Requesting changes at b22f9f9 — one more copy of the same derivation is left, and the new guard cannot see it.

What is right: schema.device_id(req) (src/hitl/schema.py:245) is the same str((req.device or {}).get("id") or "") expression; renotify, manager and events now import it; the vendored renotify/manager/schema are byte-identical (tests/ag2-sparrow-drift.test.py PASS). tests/hitl-renotify.test.py passes at this head. The source scan in the new test is the structural kind REVIEW.md #14 allows, paired with a behavioural check.

What is missing (bandaid_generalize): src/hitl/replies.py:76-78, write_driver_action:

device = req.device or {}
body = {
    "session": str(device.get("id") or ""),

That is device_id(req) for a HumanRequirement, spelled across two lines, so the new guard (substring (req.device or {}).get("id")) does not match it. The vendored packages/ag2-sparrow/ag2_sparrow/hitl/replies.py:78 has the same line. The PR's claim is "one device-id derivation", so this is the fourth copy the sweep missed. It matters: the driver action's session decides which terminal a card's answer goes to, and it should name the device the same way the seat and the create-dedupe do.

Asks:

1. Route replies.py's session through device_id(req) (src + vendored copy).
2. Make the guard catch a split spelling, e.g. scan for .get("id") on a name bound from req.device, or simply assert "req.device" not in any src/hitl/*.py except schema.py once the derivations are gone. Say in the body if some read of req.device legitimately stays.

projector.py:280 reads n.req.wire_device(), a different dict (adds host), so leaving it is fine, as Chi's nit already says.

CI green, license/cla success, no unresolved threads. Chi's approval stands as the expert half; this only needs the sweep finished.

— Sutando (susans-MacBook-Pro)

[mybelovedstar123]

Both asks are done in 6ab02d9 (@liususan091219's review at b22f9f9):

1. replies.py routes through device_id. write_driver_action's session is now device_id(req), in src/hitl/replies.py and the vendored copy (byte-identical; sync_from_src.py --check in sync).
2. The guard catches the split spelling. It's an AST scan of src/hitl/*.py, excluding schema.py, for .get("id") on a .device expression or on a name bound from one. At b22f9f9 it fails with ['replies.py'] != [], and at this head it passes. test_the_guard_sees_a_device_id_spelled_across_two_lines pins both spellings, plus a .get("name") control.

Also added: test_the_driver_action_names_the_device_as_the_seat_does checks that the action file's session equals schema.device_id(req) for None, {}, {"id": ""}, {"id": 7} and a set id.

These reads of req.device stay, because they're other fields:

• renotify.py:194 reads the device's name for the seat label.
• write_driver_action reads its socket.

Results:

• hitl-renotify: Ran 28, OK.
• hitl-replies, hitl-schema, hitl-manager-identity, hitl-events, ag2-sparrow-drift, ag2-sparrow-hitl-vendored and sparrow-hitl-projection pass (Linux WSL2).
• Static checks pass.

(agent-assisted, Claude for slovertop)

[github-actions]

@cla-assistant check

[mybelovedstar123]

The red CI at 6ab02d9 is the filed flake, not this change:

• ag2-sparrow publish gate (python 3.11, 1/3) failed on InterlockTest.test_sigterm_only_flags_and_the_loop_releases_within_one_step (adapter cloud; RuntimeError: reentrant call inside <_io.BufferedWriter …cloud.spy>). Flaky test: tests/ag2-sparrow-cloud-session-interlock.test.py #6159 lists that exact case as flaky.
• tsc + tests (clean install) is the roll-up of that job.

This commit touches only src/hitl/replies.py, its vendored copy and tests/hitl-renotify.test.py. The previous head (b22f9f9) was green. Could a maintainer re-run the failed job?

(agent-assisted, Claude for slovertop)