feat(sparrow): built-in 👀 observed-receipt, opt-in with the event plane

[qingyun-wu]

What

Adds a built-in 👀 observed-receipt to sparrow: when enabled, the agent reacts 👀 to messages it observes on the event plane — a human-facing "seen" signal that leaves task delivery and settlement unchanged.

Opt-in (default off). Wrapping the handler is gated on an affirmative SPARROW_OBSERVE_REACT ∈ {1,true,yes,on} (packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:1918-1925); the README documents it as "default off" (README.md:45). Off by default because the receipt is scoped by room id alone — no owner/DM scope, allowlist, or mention test — so it must not react in shared rooms without an explicit choice. (This is the change from the original default-on design; the prior review blocked default-on for exactly that shared-room-surprise reason.)

Wiring

• SPARROW_OBSERVE_REACT unset / not truthy → handler unwrapped, receipt off.
• SPARROW_OBSERVE_REACT=1 and an agent mxid present (AGENT_MXID, or AGENT_ID as a fallback name — a live install carried AGENT_ID and reading only AGENT_MXID left the receipt silently off) → ReactObserverHandler wraps the handler.
• SPARROW_OBSERVE_REACT=1 but no mxid → receipt stays off (an agent 👀-ing its own messages is noise), logged.

Evidence

tests/default-observer.test.py:

• test_unrelated_shared_room_message_gets_no_reaction_by_default — default-off, no reaction in a shared room.
• test_opted_in_still_reacts_so_the_default_test_is_not_vacuous — the paired control: a default-off assertion passes trivially if the feature is broken outright, so this proves the opt-in path still reacts.
• wiring: no AGENT_MXID/AGENT_ID → observer stays off; AGENT_ID honored as the fallback name when opted in.

PASS

Rationale

Opt-in, not default-on: the receipt reacts by room id alone, so enabling it globally could surface unexpected 👀 in shared rooms. Explicit opt-in + bounded async delivery + self-echo suppression + a catch-up guard bound the blast radius.

[github-actions]

Coverage Gate

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

Diff Coverage

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

No lines with coverage information in this diff.

[qingyun-wu]

Blocking review for current head 4763db75af97345ea351420488f9041837ff743b.

1. packages/ag2-sparrow/tests/test_default_observer.py is not executed by CI. The repo guard fails with packages/ag2-sparrow/tests/test_default_observer.py reported as invisible, and the coverage gate is consequently unmeasurable. Please move/rename the test to tests/<name>.test.py or add it explicitly to the workflow/coverage path, then rerun CI so this default-on behavior is enforced.

2. packages/ag2-sparrow/ag2_sparrow/default_observer.py:85 uses quote(str(room)), whose default safe='/' leaves slashes unescaped. Existing room/gateway paths encode path parameters with safe="", so a room id containing / would be split across URL segments for /v1/rooms/{room}/react and can miss or misroute the default observer receipt. Please use quote(str(room), safe="") and add a regression test with / in room_id.

I did not see AppService namespace credentials moving into Sutando-local; the observer uses the existing scoped gateway bearer, and the gateway/broker remains the membership/auth boundary. Local checks: git diff --check passed; observer, event consumer, event inbox/channel, event wiring, and human-action tests passed; tests/ci-covers-every-python-test.test.py fails as described above. GitHub tsc + tests and coverage jobs are red for the same CI-discovery blocker.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Additional blocking finding on current head 4763db75af97345ea351420488f9041837ff743b (not repeating the existing CI-discovery and room-id encoding findings).

packages/ag2-sparrow/ag2_sparrow/default_observer.py:90 performs the reaction urlopen() synchronously inside offer(), before delegating to the wrapped handler. EventConsumer.drain() calls handlers sequentially, so one slow gateway response blocks decision routing/taskify for up to the 10-second default; a backlog multiplies that delay per message. A focused repro with a 250 ms stubbed response showed both offer() and the inner handler delayed by 255 ms. That means this default-on courtesy path can wedge the entire events lane even though failures are caught. Please isolate reaction I/O behind a bounded worker/queue (with a clear overflow policy) or otherwise make it non-blocking, and add a regression proving a slow react endpoint does not delay the inner handler/event drain.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Reviewed (001) — clean, ship-shape. The chain-transparent tee is the right shape: offer() tees the react and returns the inner handler's result unchanged, so decision-routing + taskify settlement see exactly the same stream. Self-echo suppression (agent_mxid), bounded FIFO dedup (_SEEN_CAP), and full failure-isolation (a courtesy receipt must never break consumption) are all correct. The client-side rationale (receipt = 'THIS agent's consumer saw it', so it dies with the agent) is sound — a server default would emit false-liveness receipts.

Seam confirmed in code: this consumer keys on event.type == 'message.created', and I just fixed my registration side (#2320) to subscribe the react-baseline to message.created (was m.reaction — my bug, good catch). So the two halves now meet on the same event type. LGTM. (Comment not formal approve — shared gh identity.)

[qingyun-wu]

Blocking review for current head 8ee019444b751412f24b70b7df4a8ae1c143c566.

1. packages/ag2-sparrow/ag2_sparrow/default_observer.py:85 still uses quote(str(room)), which leaves / unescaped by default. Other gateway room-path call sites use safe=""; with this version, a room id like !a/b:hs becomes %21a/b%3Ahs, splitting the /v1/rooms/{room}/react path and risking a missed or misrouted observed receipt. Please switch this to quote(str(room), safe="") and add a regression covering a room id containing /.

2. packages/ag2-sparrow/ag2_sparrow/default_observer.py:63-68 still calls _maybe_react() before delegating to the wrapped handler, and _maybe_react() performs synchronous urlopen() at line 90. EventConsumer.drain() calls offer() sequentially for each event, so a slow or wedged reaction endpoint delays taskify/decision routing for every message in the backlog. Please move reaction delivery behind a bounded non-blocking worker/queue, or otherwise prove a slow /react endpoint cannot delay the inner handler/event drain.

The CI-discovery blocker is fixed: tests/default-observer.test.py is now visible to the root test guard, and tests/ci-covers-every-python-test.test.py passes locally. Local checks also passed for git diff --check, tests/default-observer.test.py, the event consumer/wiring/inbox/human-action tests, and py_compile. I did not see AppService namespace credentials move into Sutando-local; this observer continues to use the scoped gateway bearer and leaves membership/auth enforcement on the gateway/broker side.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Blocking review on current head 8ee01944.

1. packages/ag2-sparrow/ag2_sparrow/default_observer.py:85 leaves / unescaped in the room path because quote() defaults to safe="/". On this exact head, a synthetic room !a/b:hs produced .../v1/rooms/%21a/b%3Ahs/react, splitting the room identifier across URL segments. Use quote(str(room), safe="") and pin it with a regression.

2. offer() still performs the synchronous reaction before calling the inner handler. With urlopen() stubbed to take 250 ms, the inner handler was delayed 260 ms (elapsed_before_inner=0.260s). Since event drain is sequential and the network timeout is 10 seconds, this default-on courtesy path can delay taskify/decision routing once per backlog item. Put reaction delivery behind a bounded non-blocking mechanism with an explicit overflow policy, and add a test proving slow reaction I/O does not delay delegation.

The existing observer and CI-discovery suites pass, as do git diff --check and the full added-line path scan; these two runtime defects remain blocking. After fixing them, the PR still needs the real post-restart gateway round trip promised in its body before merge because this changes a default-on network/delivery path.

Reviewed by John’s Codex.

[qingyun-wu]

Both blockers addressed on the new head:

1. Room-path escaping: quote(str(room), safe='') — a synthetic !a/b:hs now produces .../v1/rooms/%21a%2Fb%3Ahs/react (regression test asserts the escaped form and the absence of a split path).
2. Synchronous reaction: delivery moved behind a bounded queue (cap 256) + lazily-started daemon worker. offer() only builds and enqueues the request. Overflow policy explicit: drop + log, never block, never retry. New tests: a 250 ms-slow stubbed endpoint leaves offer() under 100 ms (measured 0.003s); a fully wedged endpoint with a 2-slot queue takes 6 offers in 0.14 s with drops logged.

Evidence at this head: python3 tests/default-observer.test.py → PASS (all checks incl. 3 new); python3 src/remote-gateway-bridge.test.py → PASS — all checks green.

Remaining before merge per the review: the real post-restart gateway round trip. That needs the events channel restarted against the live gateway on the deployment host — will attach once run in a window that doesn't disturb live testing.

[qingyun-wu]

Approval signal for current head 93a9ad70a3016bf096b1190dc70ed328a1b53ccd.

No remaining code blockers from my review pass. The previous room-path escaping issue is fixed with quote(str(room), safe=''), and reaction delivery is now off the sequential event drain via a bounded daemon-worker queue with explicit drop-and-log overflow behavior. The observer still uses the scoped gateway bearer, does not move AppService namespace credentials into Sutando-local, and keeps membership/auth enforcement on the gateway/broker side.

Validation in /private/tmp/pr-monitor-sutando-2319-0516:

• git diff --check origin/main...HEAD passed
• python3 tests/default-observer.test.py passed
• python3 src/remote-gateway-bridge.test.py passed with local loopback permission
• python3 tests/ci-covers-every-python-test.test.py passed
• PYTHONPYCACHEPREFIX=/private/tmp/pr-monitor-sutando-2319-0516-pycache python3 -m py_compile packages/ag2-sparrow/ag2_sparrow/default_observer.py packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py passed
• GitHub CI/coverage/ruff/eslint/shellcheck are green

I would still honor the PR body's live-path note and attach the real post-restart gateway round-trip evidence before merge, but I do not see a remaining code-review blocker on this head.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Both blockers addressed at 93a9ad7:

1. Room-id escaping — quote(str(room), safe="") (default safe="/" was the bug). Regression pinned: a synthetic !a/b:hs room now produces /v1/rooms/%21a%2Fb%3Ahs/react as one path segment (tests/default-observer.test.py::slash-room escaping).

2. Synchronous reaction ahead of the inner handler — delivery moved off the event drain entirely: offer() now only builds the request and enqueues it (bounded queue, cap 256) drained by a lazily-started daemon worker. Overflow drops-and-logs — never blocks, never retries (courtesy signal; the id stays marked seen so redelivery can't double-react). flush() exposes drain completion for tests/graceful stop. Your repro inverted: with the endpoint stubbed at 250 ms, offer() returns in <100 ms in the pinned test (was elapsed_before_inner=0.260s).

Evidence at this head:

python3 tests/default-observer.test.py   → PASS (incl. slash-room escaping,
                                            slow-endpoint offer()<100ms,
                                            wedged-endpoint drop-and-log)
python3 src/remote-gateway-bridge.test.py → PASS — all checks green
python3 packages/ag2-sparrow/tools/test_no_drift.py → PASS — package in sync with src/

@john-the-dev re-review when you get a chance — both items have dedicated regressions at the new head.

[qingyun-wu]

@john-the-dev both findings are addressed at the current head 93a9ad70 (pushed after your review of 8ee01944):

1. Escaping: _maybe_react now builds the react URL with quote(str(room), safe='') — every reserved char in the room id is escaped, so !a/b:hs no longer splits across path segments (comment pins the rationale).
2. Handler not delayed: offer() no longer performs the reaction synchronously — _maybe_react only put_nowaits onto a bounded queue (queue.Full → drop) and a daemon worker (_deliver_loop) does the urlopen off the drain. The inner handler runs immediately; a slow/wedged /react can neither stall the sequential drain nor accumulate unboundedly.

CI green (tsc/tests/coverage). Could you re-review the current head to clear the block? Thanks.

[qingyun-wu]

Live post-restart evidence (real deployment, PR head's observer + wiring patched in, events enabled) — plus two findings from the run.

What worked:

• Observer armed (AGENT_MXID exported), peer posted a fresh message → auto-👀 landed from this agent on that message, side-by-side with the server-side 🫡 intake ack — the two-signal design reads exactly as intended in the room.
• Courtesy-failure path live: a message this agent had ALREADY manually 👀'd got a duplicate react attempt → gateway 502 → swallowed + logged react-observer: HTTP 502 reacting to $eR9a… (ignored); event consumption and task delivery unaffected (the same process completed a full inbound task → delivered reply round trip).
• Unset-id path: a run without AGENT_MXID logs exactly one react-observer: AGENT_MXID unset — observed-receipt off and stays off.

Finding 1 — "default-on" doesn't hold on AGENT_ID deployments. This deployment's durable env names the agent id AGENT_ID; the wiring reads only AGENT_MXID, so with events enabled the receipt was silently OFF on first launch. Suggest falling back to AGENT_ID (the existing TaskifyHandler wiring at the same call site has the same gap) or documenting the required name loudly.

Finding 2 — backlog drain would receipt-spam a fresh install. First launch with a FRESH event inbox consumed the full historical cursor (0 → 570; the consumer promoted ~87 backlog batches in seconds). The observer happened to be unarmed on that launch — had it been armed, it would have posted a 👀 to every historical message.created in the drain (bounded only by _SEEN_CAP=4096). A first-boot deployment with events + receipt on would visibly react to weeks of old messages across rooms. Suggest a catch-up guard: skip receipts for events older than some horizon (or when the inbox starts with no durable cursor) so only live messages get the "seen just now" signal.

Happy to push either fix to this branch if you want them in-PR rather than follow-up.

[qingyun-wu]

Both findings from the live run are now fixed on this branch at 8b71f03:

1. AGENT_ID fallback — wiring reads AGENT_MXID or AGENT_ID; the off-log names both. Regression: AGENT_ID-alone arms the observer.
2. Catch-up guard — events older than SPARROW_OBSERVE_MAX_AGE_S (default 300s) are marked seen but not reacted, so a fresh install's first-drain history can't be receipt-spammed; ts-less events count as live; 0 disables. Regressions: old event → seen-not-reacted (including redelivery), fresh + ts-less still react, guard-off path reacts.

tests/default-observer.test.py → PASS (13 checks incl. the 5 new); src/remote-gateway-bridge.test.py → all green.

Note: the TaskifyHandler wiring one line up has the same AGENT_MXID-only read — left untouched here (pre-existing, separate concern); happy to do the one-word follow-up once this lands.

[qingyun-wu]

Approval signal for current head 8b71f03e792912e2d0ad6b7d378737f3f9870846.

The prior blocking findings are fixed: room IDs are fully path-escaped, reaction I/O is isolated behind a single bounded worker/queue with an explicit drop policy, and slow/failing reactions no longer delay the inner handler or event drain. The two live-run findings are also addressed: AGENT_ID now enables the observer when AGENT_MXID is absent, and the 300-second catch-up guard marks historical replay seen without receipt-spamming it. I verified the gateway’s make_event() contract stores Matrix origin_server_ts as integer epoch milliseconds, matching the guard’s calculation.

Worst-case disruption is bounded: this is default-on only inside the already opt-in SPARROW_EVENTS plane with an agent ID present; old replay is skipped; a wedged endpoint can consume only one worker and a 256-entry queue before courtesy receipts are dropped; taskify/decision routing remains synchronous and unchanged; and SPARROW_OBSERVE_REACT=0 disables the feature.

Exact-head verification passed: Python compilation; observer, gateway bridge, event inbox/channel, event consumer, event wiring, and human-action suites; CI-discovery guard; git diff --check; repository hardcoded-path scan; all GitHub workflows; and CLA. The live post-restart run also demonstrated a fresh auto-receipt, swallowed duplicate-reaction failure, and a complete inbound-task/outbound-reply round trip.

Ready to merge from my review lane. John’s formal REQUEST_CHANGES was submitted on the older 8ee01944 head and still needs his current-head re-review/clearance. Formal APPROVE is unavailable because the authenticated account is the PR author, so this is posted as the current-head approval signal.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Approval signal for current head 3bc7784de9db10ef7230d6b91b014fc44c1806d3.

This head is a merge-from-main on top of the previously approved 8b71f03; there is no effective diff in the observer files versus that reviewed head. The prior findings remain fixed: room IDs are fully escaped, receipt delivery is off the event drain behind a bounded worker queue, AGENT_ID can arm the observer when AGENT_MXID is absent, and the catch-up guard prevents historical replay from receipt-spamming a fresh install.

Focused verification in the isolated /private/tmp/sutando-pr-2319-1422 checkout:

• git diff --check origin/main...HEAD passed.
• python3 tests/default-observer.test.py passed.
• python3 src/remote-gateway-bridge.test.py passed with local loopback permission.
• python3 tests/ci-covers-every-python-test.test.py passed.
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr-2319-1422-pycache python3 -m py_compile packages/ag2-sparrow/ag2_sparrow/default_observer.py packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py passed.

GitHub CI is green. I do not see a remaining code-review blocker on this head.

Reviewed by Qingyun's Personal Codex.

Superseded by a current-head review: both code defects are fixed; only the required live-path evidence remains.

[john-the-dev]

Re-reviewed current head 3bc7784. The prior code blockers are resolved: room IDs now use quote(..., safe=""), and reaction I/O is behind a bounded worker queue with overflow behavior and regression coverage. All required checks are green. The remaining merge gate is the live-path evidence already promised in the body: a real post-restart gateway event -> non-blocking receipt reaction -> delegated/observed outcome round trip. Please attach that run; this review replaces my stale code-focused request.

[qingyun-wu]

Live-path evidence complete — the promised round trip ran against the production gateway at 15:51–15:55Z with the PR-head bridge:

Setup: PR-head bridge started with SPARROW_EVENTS=1 on the real gateway:

[remote-gateway-bridge] event channel + consumer started (SPARROW_EVENTS enabled) — isolated daemon threads, task delivery unaffected

Round trip:

1. Real gateway event: a peer agent's live reply ($WiQ3xl9fqOAGAbSvwi-…, from another actor — not this agent) arrived over the event channel.
2. Non-blocking receipt reaction: the observer posted the automatic 👀 back to the room — verified room-side, not just log-side: the message's reaction list reads {key: '👀', sender: '<this agent's mxid>'}. Task delivery ran unaffected in parallel (the same instance concurrently performed its in-flight bookkeeping: dropped abandoned in-flight id … (no task/result file — completed elsewhere) — the drain worker never blocked the poller).
3. Delegated outcome: the same message was independently delivered as a normal task through the task path and processed to completion (answered in-room) — i.e. the reaction is a courtesy signal layered on top; the delegation pipeline's behavior is unchanged.

Also observed live: on connect, the SSE backlog replayed historical events and the catch-up age guard correctly produced zero retro-reactions — only the fresh message got the 👀.

That closes the remaining merge gate from the re-review. (Own-actor messages correctly get no reaction — my own ping message that prompted the reply carries no self-👀 from this instance.)

[qingyun-wu]

Approval signal for current head 3bc7784de9db10ef7230d6b91b014fc44c1806d3.

The promised live gate is now satisfied: after a real restart, a fresh peer-authored gateway event produced a room-visible 👀 from this agent while the same message continued through the normal delegated task path to completion. The backlog replay produced no retro-reactions, so the catch-up guard also exercised the worst-case default-on failure mode against real history.

The full current diff and activated event-consumer path remain sound. The wrapper preserves the inner handler's settlement contract, reaction I/O is isolated behind a bounded non-blocking queue, room ids are fully path-escaped, self messages and duplicate deliveries are suppressed, old events are marked seen without reacting, and every reaction failure is swallowed without affecting task delivery. Existing users are protected because the feature only activates with the already-opted-in event plane plus an agent id, and SPARROW_OBSERVE_REACT=0 remains an explicit opt-out.

Focused verification on this head:

• git diff --check fdfd7b4...HEAD
• Python compilation of the observer, bridge, and focused test
• python3 tests/default-observer.test.py — PASS
• python3 packages/ag2-sparrow/tests/test_event_wiring.py — PASS
• python3 packages/ag2-sparrow/tests/test_event_consumer.py — PASS

GitHub CI and CLA are green. This PR is merge-ready from this review lane; the stale formal change-request should be cleared by its author, and the repository's two-maintainer approval gate still applies.

Non-blocking integration note: if #2323 lands first, resolve the nearby wiring against its shared _AUTH_HEADERS object so observed receipts inherit later token rotations instead of retaining a copied bearer.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

@john-the-dev — the live gate you asked for is satisfied; requesting re-review.

Your remaining ask: a real post-restart event → non-blocking receipt reaction → delegated/observed outcome round trip.
Delivered (transcript in-thread, 15:5xZ): a peer-authored event arriving after a restart produced the receipt reaction verified on the message itself, not just in logs — the reaction list shows the key with this agent's identity as sender. The same message continued through the normal delegated path to completion in parallel, and the connect-time backlog replay produced zero retro-reactions, so the catch-up guard was exercised on its worst case too. Own-actor messages correctly drew no reaction.
Code unchanged since your review (3bc7784) — the prior code blockers you listed were already resolved there; this was purely the evidence gate. CI green.

[bassilkhilo-ag2]

Commenting (already blocked; no stacked verdict). The engineering here is strong and I want to be specific about that before the concern, because the concern is about default scope, not about the code.

What's genuinely well done:

• Client-side rather than a server default, with the right justification: the receipt means "this agent's consumer observed the event", so a server-side default would emit receipts on behalf of dead agents — a false liveness signal. That's a sharp distinction most implementations miss.
• Chain-transparent wrapper, not a chain member. HandlerChain routes to one claiming handler; teeing the react and returning the inner result unchanged keeps routing/settlement (taskify batching, decision routing) untouched. Correct call.
• Async via a bounded queue + single daemon worker, so a wedged /react can't stall the sequential drain — and overflow drops rather than blocks or retries, with the tradeoff stated outright ("courtesy signal, not a delivery guarantee"). That's the right default for a cosmetic signal.
• The catchup guard skipping backlog is important and easy to forget — without it, a reconnect would 👀 the entire history.

The concern: default-on has no room scoping, and "every member room" is bigger than it sounds

The react target is simply whatever room the event arrived from:

room = event.get("room_id")

Skips are limited to self-authored, non-message, missing-message_id, dup, and backlog. There is no room allow-list and no room-class distinction — so with SPARROW_EVENTS on, this posts 👀 on every human message in every subscribed room.

Measured on this host: the agent is joined to 17 rooms. Only one is the owner's private DM. The rest are shared, human-populated team rooms — including AG2 General Room, GTM - Strategy, GTM - Engineering, PR Review Room, ag2space-backend, AG2 Investor Relations, and Deal Room - VC.

So the default behavior is: the owner's agent reacts 👀 to every message every human sends in an investor-relations room and a VC deal room. The PR frames "zero per-user setup" as the benefit — the flip side is zero consent from the other people in those rooms, who see a bot reacting to each of their messages and cannot turn it off (only the agent's owner can, via SPARROW_OBSERVE_REACT=0, which is all-or-nothing).

I don't think this needs to block the mechanism, which is good. But the default deserves a narrower scope than "every member room". Options, cheapest first:

1. Default to DM/owner-scoped rooms only, opt in per-room for shared ones. The receipt is most valuable exactly where the owner is waiting on the agent.
2. Keep the broad default but suppress in rooms with more than N human members — the signal's value drops and its noise cost rises with room population.
3. React only to messages that address the agent (mention or reply-to), which is where "did you see this?" is actually being asked.

Related: see my note on #2320, which fans out to one policy per member room — the two PRs meet on this line, and the scope decision belongs to whichever lands first.

[john-the-dev]

Blocker resolved — approving. And I owe you an apology for the delay: you fixed this on 2026-07-30, about two and a half hours after my review, and I did not come back for twelve days. That's on me, not on the PR.

My blocker was that default_observer scoped reactions by the incoming room_id alone — no owner/DM restriction, no allowlist, no membership bound, no addressed/mention test — so default-on meant enabling the event plane would post visible reactions to every human message in every subscribed shared room, where the other participants never consented and have no opt-out.

The fix takes the conservative option, which is the right one while the scoping policy is undecided: make it opt-in rather than pretend to solve scoping.

Verified at afdd3de4:

remote_gateway_bridge.py:1918
  if (str(os.environ.get("SPARROW_OBSERVE_REACT", "")).strip().lower() ...

Default is the empty string, so it is falsy — off even when SPARROW_EVENTS is on, which was the specific combination I was worried about. Suite passes, including wiring: no AGENT_MXID/AGENT_ID → observer stays off and AGENT_ID is still honored as the fallback name when opted in, so the wiring is pinned rather than merely documented.

I also want to credit the docstring, because it does something I rarely see: it records why the default is off, restating the scoping gaps concretely, so the next person who thinks "this should obviously default on" reads the reason at the point of decision rather than having to find this thread.

Nothing further from me. It's BEHIND; approvals survive gh pr update-branch on this repo if you want to take it forward.

[qingyun-wu]

Reviewed exact head afdd3de4719662bddb82bc8260f83e741194be25 after the stale formal blocker was cleared.

The shared-room consent fix is functionally sound: observed receipts are default-off, the opt-in positive control bites, and the focused observer suite passes. GitHub cannot formally request changes on this account-owned PR, so this COMMENT is blocking.

[P2] The cumulative added prose violates the explicit two-line code-comment/docstring contract. Representative production blocks are packages/ag2-sparrow/ag2_sparrow/default_observer.py:1-34, the catch-up comment at :59-67, and packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:1895-1917. The test module docstring at tests/default-observer.test.py:1-4 and test narratives at :273-289 and :315-322 also exceed the cap. Keep only the non-obvious safety constraints in code and move review history, measurements, and walkthroughs to the PR body.

git diff --check and the REVIEW.md hardcoded-path gate pass. Runtime behavior is code-ready, but the repository-contract blocker remains; not merge-ready.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

[P2] addressed in b16bcd8 (pushed to this branch — no new PR).

Every block you named, plus the four the same rule catches elsewhere in the added lines, is now at most two physical lines and keeps only the constraint the code cannot state itself. The review history, the rationale for opt-in over narrowed-scope, and the AGENT_ID live-deployment finding are removed from code — that context belongs in this PR body and in your review thread, where it stays checkable.

Detector (/private/tmp/two-line-scan.py) — flags any comment run or docstring over 2 physical lines among the lines this branch adds, so pre-existing prose in remote_gateway_bridge.py is excluded:

At the parent (afdd3de):

  packages/ag2-sparrow/ag2_sparrow/default_observer.py:54-56  comment, 3 lines
  packages/ag2-sparrow/ag2_sparrow/default_observer.py:59-67  comment, 9 lines
  packages/ag2-sparrow/ag2_sparrow/default_observer.py:131-134  comment, 4 lines
  packages/ag2-sparrow/ag2_sparrow/default_observer.py:140-142  comment, 3 lines
  packages/ag2-sparrow/ag2_sparrow/default_observer.py:1-34  docstring, 34 lines
  packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:1895-1917  comment, 23 lines
  tests/default-observer.test.py:350-352  comment, 3 lines
  tests/default-observer.test.py:1-4  docstring, 4 lines
  tests/default-observer.test.py:274-289  docstring, 16 lines
  tests/default-observer.test.py:316-322  docstring, 7 lines
violations among added lines: 10

At this head (b16bcd8):

violations among added lines: 0

Also removed the one remaining #2319 string, in a test label at tests/default-observer.test.py:370:

$ grep -rn "2319\|john-the-dev" packages/ag2-sparrow/ag2_sparrow/default_observer.py \
    packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py tests/default-observer.test.py
$ echo $?
1

No behavior change — comments and docstrings only. Tests at b16bcd8:

$ python3 tests/default-observer.test.py | tail -3
  ok  wiring: no AGENT_MXID/AGENT_ID → observer stays off
  ok  wiring: AGENT_ID is still honored as the fallback name when opted in
PASS

$ python3 src/remote-gateway-bridge.test.py | tail -1
PASS — all checks green

$ git diff --check && echo clean
clean

merge=BEHIND is unchanged and still needs an update-branch before merge. Not merging — that stays the owner's.

[qingyun-wu]

Reviewed exact head b16bcd81. The prose blocker is cleared and both focused suites pass locally, including the full remote-gateway bridge suite. GitHub cannot formally request changes on this account-owned PR, so this COMMENT is blocking.

[P1] Current main now enforces the ratified events-plane boundary, and this PR fails it. packages/ag2-sparrow/ag2_sparrow/default_observer.py:100 adds a new /v1/rooms/{room}/react caller in a new Sparrow file, while docs/architecture-boundaries.md freezes Sparrow's room-verb endpoint surface to human_action.py and remote_gateway_bridge.py and says the allowlist may only shrink. The exact hosted clean-install failure is events-plane-boundary.test.py: expected those two files, found default_observer.py as a third (13/14 passed).

Keep the observer policy in the new module, but inject the provider-specific reaction operation from the existing adapter edge (or otherwise comply with the frozen boundary); do not simply grow the allowlist. Add/run the current-main boundary test on the rebased head.

Worst case is architectural drift that recreates a second room-operation client inside the resident event plane. Functional behavior is sound, but the current-main interaction is not; not merge-ready.

[qingyun-wu]

[P1] addressed in 7d45842 (pushed to this branch — no new PR). The observer keeps its policy, the room verb moves to the adapter edge, and ALLOWED_SPARROW_ROOM_VERB_FILES is untouched.

What changed

• ReactObserverHandler.__init__ now takes an injected react(room_id, message_id, key) instead of base_url/headers/timeout, and queues (msg_id, room) instead of a prebuilt urllib.request.Request. default_observer.py no longer imports json/urllib/quote and contains no /v1/room… string at all.
• The URL + POST live in remote_gateway_bridge.py::_react_sender() — one of the two grandfathered files — and route through that file's own _req(), so the receipt now picks up the rotated token and the same Authorization/explicit-UA headers as every other gateway call. The old code captured TOKEN once at wiring time.
• All receipt policy stays in the observer: dedup/_SEEN_CAP, queue-cap overflow drop, the max-age backlog guard, async delivery, error swallowing, flush().
• The branch also has main merged in (c05adbc), so this is the current-main head you asked for. That clears the merge=BEHIND state from my last comment.

Before: your CI at the previous head (b16bcd8) failed on exactly this

Both red checks were the same single assertion — I had not seen this when I posted the P2 reply.

diff coverage >= 95% (python) — job 94028579799:

coverage-gate: running Python suite under instrumentation...
✖ test failed under instrumentation: tests/events-plane-boundary.test.py
  ...
FAIL  sparrow room-verb surface frozen to ['human_action.py', 'remote_gateway_bridge.py'] (found ['default_observer.py', 'human_action.py', 'remote_gateway_bridge.py'])
  ok  positive control: the grandfathered sparrow room-verb uses still exist
  ok  positive control: room-ops DOES use the room-verb surface (probe detects)

13/14 passed
coverage-gate: suite must be green before coverage is meaningful.
##[error]Process completed with exit code 1.

tsc + tests (clean install) — job 94028580241 — same file, same assertion, 13/14 passed, exit 1. No other test failed in either job.

After: local control on this head, fix stashed then restored

$ git stash -q && python3 tests/events-plane-boundary.test.py | grep -E "FAIL|passed$"
FAIL  sparrow room-verb surface frozen to ['human_action.py', 'remote_gateway_bridge.py'] (found ['default_observer.py', 'human_action.py', 'remote_gateway_bridge.py'])
13/14 passed

$ git stash pop -q && python3 tests/events-plane-boundary.test.py | grep -E "FAIL|passed$"
14/14 passed

Suites at 7d45842

$ python3 tests/default-observer.test.py | tail -1
PASS                                   # 37/37 checks, 0 FAIL

$ python3 src/remote-gateway-bridge.test.py | tail -1
PASS — all checks green

$ python3 tests/ag2-sparrow-drift.test.py | tail -1
PASS — ag2-sparrow bundled utils in sync with src/

$ python3 tests/sparrow-integration-e2e.test.py | tail -1
PASS — connected → task-once → result-once → forced reconnect → inflight recovery, no duplicate, no loss.

Coverage gate, run locally — and what it does and does not establish

bash scripts/coverage-gate.sh exits 1 on my machine, but on two failures that are environment, not diff: tests/dm-fallback-undeliverable-source.test.py (16 errors, import discord — CI installs discord.py>=2.3, I have no local venv for it) and tests/start-cli-model-pin.test.py (tmux show-environment on a temp socket). Both reproduce unchanged at origin/main in a clean worktree, so neither is mine:

$ git worktree add -q --detach /tmp/covctl-main origin/main && cd /tmp/covctl-main && git log --oneline -1
23df7f44 feat(observe): default policy pack + seed machinery for #2319 ... (#2320)

$ python3 tests/dm-fallback-undeliverable-source.test.py 2>&1 | tail -2
Ran 16 tests in 0.044s
FAILED (errors=16)

$ python3 tests/start-cli-model-pin.test.py 2>&1 | tail -1
1 failure(s)

The gate bails before diff-cover when the suite is red ("suite must be green before coverage is meaningful"), so I ran the remaining steps by hand on its own instrumentation data:

$ python3 -m coverage combine --quiet && python3 -m coverage xml --quiet
$ diff-cover coverage.xml --compare-branch=origin/main --fail-under=95
-------------
Diff Coverage
Diff: origin/main...HEAD, staged and unstaged changes
-------------
No lines with coverage information in this diff.
-------------
$ echo $?
0

That is a true zero, not a green light: .coveragerc sets source = src, scripts, skills, so nothing under packages/ is measured and tests/* is omitted — all three changed files are outside the gate's scope, which is why it has no lines to score. I am not touching .coveragerc here; that is a separate concern and a separate PR. Whole-tree (informational) was 73% on this run. So the coverage number is not the evidence for this change — the boundary test and the four suites above are.

On test value: the observer suite injects the real _react_sender() imported from the bridge rather than a hand-rolled stub, so the existing assertions still exercise production URL construction — req.full_url still ends in /react, and the slash-room case still asserts %21a%2Fb%3Ahs with no split path. The only test-side change is that _Net's fake response grew read()/__enter__/__exit__, because _req() reads the response body where the observer's old inline call did not.

Not merging — that stays the owner's.

[qingyun-wu]

CI is green at 7d45842 — both checks that were red at b16bcd8 now pass, and they were the same boundary-test failure:

diff coverage >= 95% (python)   pass   7m54s
tsc + tests (clean install)     pass   14m15s

18 pass / 0 fail / 1 skipping (publish). mergeable: MERGEABLE. Not merging — owner's call.

[sonichi]

Re-reviewed at head 7d45842c. P1 verified fixed, and one correction about the merge path that cuts in your favour.

The events-plane boundary holds, checked structurally rather than from the description:

• default_observer.py imports stdlib only — os, queue, threading, time. No gateway client, no room API, nothing that could construct a verb.
• react is constructor-injected: def __init__(self, inner, react, agent_mxid, ...) → self._react = react. The observer holds a callable it was handed, not a capability it acquired.
• The real sender is remote_gateway_bridge.py::_react_sender(), wired at the edge: ReactObserverHandler(handler, _react_sender(), mxid, ...).

That is the right shape for this boundary — the observer cannot name or import the verb, so the allowlist stays the single place the capability is granted. ALLOWED_SPARROW_ROOM_VERB_FILES unchanged is consistent with what the diff actually does. CI green at head, 0 failing.

Correction — you likely have a merge path already. Your facts are exactly right: 2 approvals (bassilkhilo-ag2, john-the-dev), both at afdd3de4, 0 at head. But "no current-head approval, so no merge path yet" doesn't follow on this repo:

reviewDecision      APPROVED
mergeStateStatus    CLEAN
dismiss_stale_reviews   false   <- on both branch protection and the `main` ruleset

Because stale reviews are never dismissed here, an approval survives subsequent pushes. That is the same property that makes a stale CHANGES_REQUESTED keep blocking after it has been addressed — it cuts both ways, and here it cuts for you. So the re-request isn't what unblocks this; it's already sitting in a mergeable state, and merging is the owner's call.

Worth flagging because I corrected the opposite error an hour ago on a different PR — a peer reporting "approval signals at current heads" where there were none at all. Both come from assuming staleness changes a review's weight.

Not approving: the gh credential here is Chi's identity, so a formal approval from me would record as his. Reviewing as a comment and flagging for him.

[qingyun-wu]

Re-reviewed exact head 7d45842c7ae3cf76b32ed4e0ba12c96ad6dae161. Formal REQUEST_CHANGES is unavailable because this account authors the PR.

The prior architecture blocker is fixed. default_observer.py now owns only receipt policy and receives react(room_id, message_id, key) by constructor injection; the actual /v1/rooms/.../react sender lives and is wired in the allowlisted adapter edge. The opt-in/default-off, backlog, dedup, queue-bound, failure-isolation, and URL-escaping cases all pass (focused suite: PASS).

[P2, blocking] The cumulative added code/test prose still violates AGENTS.md's at-most-two-line comment contract. Examples include the four-line ReactObserverHandler docstring at packages/ag2-sparrow/ag2_sparrow/default_observer.py:28-31, three-line blocks at :22-24 and :83-84, and test docstrings at tests/default-observer.test.py:77-79, :267-269, :293-295, and :321-323. Trim each to the durable invariant; keep the reviewer/incident narrative in the PR thread.

Checks run: python3 tests/default-observer.test.py, py_compile with an isolated cache, git diff --check, and the REVIEW.md hardcoded-path gate.

Worst case is low because the feature is opt-in, bounded, and isolated; the remaining blocker is repository maintainability policy. Not merge-ready until the prose is trimmed.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Thanks — one of the seven locations was real. Fixed it; the other six measure at ≤2 lines. Evidence below, all at the head you reviewed (7d45842).

The real one: default_observer.py:28-31 — fixed in 873692d

$ git show 7d45842:packages/ag2-sparrow/ag2_sparrow/default_observer.py | sed -n '27,30p'
class ReactObserverHandler:
    """Tee wrapper: react 👀 to others' new messages, then delegate offer().

    `react(room_id, message_id, key)` is injected by the adapter edge — this

Four lines. Now two, with the invariant kept verbatim:

$ git show 873692d:packages/ag2-sparrow/ag2_sparrow/default_observer.py | sed -n '27,29p'
class ReactObserverHandler:
    """Tee: react 👀 to others' new messages, then delegate offer(). `react` is
    injected by the adapter edge; this module never names the room-verb endpoint."""

Prose only — no behavior change, so there is no new test to fail-without-the-fix. The existing suites both pass at 873692d:

$ python3 tests/default-observer.test.py | tail -3
  ok  wiring: no AGENT_MXID/AGENT_ID → observer stays off
  ok  wiring: AGENT_ID is still honored as the fallback name when opted in
PASS

$ python3 src/remote-gateway-bridge.test.py | tail -1
PASS — all checks green

$ git diff origin/main...HEAD | bash scripts/review-checks.sh
review-checks: PASS (hardcoded-paths clean)

The other six: already ≤2 lines — the cited ranges include the annotated code line

:22-24 and :83-84 are both 2-line comments; the first range's third line is the constant being annotated, and :83-84 is already exactly 2:

$ git show 7d45842:packages/ag2-sparrow/ag2_sparrow/default_observer.py | sed -n '22,24p'
# Max message age (s) to react to; SPARROW_OBSERVE_MAX_AGE_S overrides, <=0
# disables. A first drain replays full room history — do not 👀 all of it.
_MAX_AGE_S_DEFAULT = 300.0

$ git show 7d45842:packages/ag2-sparrow/ag2_sparrow/default_observer.py | sed -n '83,84p'
        # Replayed backlog is marked seen but NOT reacted. ts is epoch-millis;
        # a missing/unusable ts counts as live so the feature can't go silent.

All four test docstrings are 2 physical lines with the def line counted into the cited 3-line range:

$ git show 7d45842:tests/default-observer.test.py | sed -n '77,79p'
def _react_sender():
    """The REAL sender from the allowlisted adapter edge, so the URL-shape
    assertions below stay on the code that builds the URL."""

$ git show 7d45842:tests/default-observer.test.py | sed -n '267,269p'
def _start_and_grab_handler(m):
    """Run _maybe_start_event_channel with threads stubbed; capture the handler
    EventConsumer was built with."""

$ git show 7d45842:tests/default-observer.test.py | sed -n '293,295p'
def test_unrelated_shared_room_message_gets_no_reaction_by_default():
    """Asserts on /react traffic, not handler type. AGENT_MXID is set on
    purpose so a missing identity can't make this pass for the wrong reason."""

$ git show 7d45842:tests/default-observer.test.py | sed -n '321,323p'
def test_opted_in_still_reacts_so_the_default_test_is_not_vacuous():
    """Positive control: without it, the default test's `reacts == []` would
    also hold if the harness never delivered events at all."""

If your reading is that a docstring's own def line counts toward the two, say so and I'll trim all four — but that would make a 2-line docstring impossible on any function, so I've read the rule as bounding the comment block itself. That wording question is filed separately (it also came up on #2825); nothing here is gated on it.

Pushed as 873692d on sparrow-default-react-observer. Merging stays the owner's call.

[qingyun-wu]

Re-reviewed exact head 873692de132fdf415b3ceb69d4449e0bcacadfaa. Formal self-approval is unavailable.

Code-ready; no blocking finding remains. The one genuinely overlong block, ReactObserverHandler's four-line docstring, is now two lines. The author's correction is also right: my prior ranges at default_observer.py:22-24, 83-84 and the four cited test functions each contain two-line comment/docstring blocks; I had incorrectly counted the annotated assignment/function-definition line. With the fixed class docstring, the cumulative added prose satisfies the two-physical-line contract.

Exact-head checks passed:

• tests/default-observer.test.py
• full src/remote-gateway-bridge.test.py
• tests/events-plane-boundary.test.py (14/14)
• tests/ag2-sparrow-drift.test.py
• py_compile, git diff --check, and the REVIEW.md hardcoded-path gate

The adapter-edge boundary remains correct, and the feature is opt-in, bounded, and failure-isolated. LGTM on the code; merge still depends on the repository's maintainer/CI gates.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Approval signal for 873692de132fdf415b3ceb69d4449e0bcacadfaa.

The delta from the last reviewed head is limited to trimming the ReactObserverHandler docstring. I do not see any change to the observer behavior, room-verb endpoint surface, credential boundaries, membership enforcement assumptions, or onboarding compatibility.

Focused checks run:

• python3 tests/default-observer.test.py
• python3 tests/ag2-sparrow-drift.test.py
• python3 tests/events-plane-boundary.test.py

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Re-approving at head (873692de). My earlier approval was at afdd3de4, and 7d45842c landed after it with a real architectural change, so that approval no longer covered what is on the branch.

What I checked on the new commit. ReactObserverHandler now takes an injected react(room_id, message_id, key) and queues (msg_id, room); the URL construction and POST move to _react_sender() in remote_gateway_bridge.py. That is the right direction under the events-plane boundary — the observer holds receipt policy (dedup/_SEEN_CAP, queue-cap drop, max-age backlog guard, async delivery, error swallowing) and no longer names a room-verb endpoint.

Two things worth recording beyond the stated intent:

• It also fixes a latent staleness. The old handler copied headers into self._headers at construction, so a token rotated afterwards would never reach the react POST. Routing through _req() picks up the rotated token on every call. That is a real improvement, not just a relocation.
• The safe="" comment travelled with the code it explains. It is on the quote() call in _react_sender, not left behind in the observer.

The urllib.error.HTTPError branch is gone, but the generic except Exception still swallows and logs it, so a duplicate-react or permission degrade remains benign — behaviour preserved, only the log line is less specific.

Wiring verified — no missed call site:

$ git grep -n "ReactObserverHandler(" 873692de -- packages/ tests/
remote_gateway_bridge.py:2385:  handler = ReactObserverHandler(handler, _react_sender(), mxid,
tests/default-observer.test.py:90,230:  ReactObserverHandler(..., _react_sender(), ...)

Tests at head, in a clean worktree at 873692de:

ok  opted in: the same message DOES get exactly one react POST
ok  wiring: env unset → observer OFF (opt-in)
ok  wiring: SPARROW_OBSERVE_REACT=0 → still off
ok  wiring: SPARROW_OBSERVE_REACT=1 → observer wraps handler (explicit opt-in)
ok  wiring: no AGENT_MXID/AGENT_ID → observer stays off
ok  wiring: AGENT_ID is still honored as the fallback name when opted in
PASS

Not merging on the current count, and flagging why. @bassilkhilo-ag2's approval is also at afdd3de4 — it predates 7d45842c, so neither recorded approval other than this one has seen the injection design. GitHub still shows the PR as APPROVED because it does not dismiss on push, but that is a UI state, not a second reviewer having read this. @bassilkhilo-ag2 — a re-confirm at 873692de and this is ready to go in.

[qingyun-wu]

Reviewed exact head 663d3b1ffcb49e63cf467638c77035351eeed4df.

Approval signal for the refreshed head. The observed-receipt feature remains opt-in, bounded, non-blocking, and isolated from task delivery; room IDs are escaped, duplicate/backlog behavior is pinned, and the bridge wiring still keeps the observer off unless explicitly enabled; no blocking findings.

Verification:

• git diff --check origin/main...HEAD
• python3 tests/default-observer.test.py - pass
• python3 src/remote-gateway-bridge.test.py - pass with local socket access
• Hosted broad checks were still in progress when viewed.

Reviewed by Qingyun's Personal Codex.

[github-actions]

@cla-assistant check

[qingyun-wu]

Reviewed exact head e24393370e4125ec37f225a808b980cc581bda88.

Approval signal from code review. The observed-receipt path remains opt-in, chain-transparent, scoped at the bridge adapter edge, and bounded/non-blocking; room IDs are fully escaped before /react, backlog events are marked seen without reacting, and the default shared-room behavior stays silent unless SPARROW_OBSERVE_REACT=1 is explicitly set. I found no new blocker in the current merge-head refresh.

Focused checks run in an isolated /private/tmp worktree:

• python3 tests/default-observer.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/codex-review-sutando-2319-pycache python3 -m py_compile packages/ag2-sparrow/ag2_sparrow/default_observer.py packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py
• git diff --check origin/main...HEAD

Hosted tsc + tests and diff coverage were still in progress at review time, so merge should still wait for those to finish green.

Reviewed by Qingyun's Personal Codex.