[qingyun-wu]

Problem

The repo contract says "write results/proactive-{ts}.txt to speak to the user" — but only the Discord/Telegram bridges drain that path. On a gateway-only host, proactive nudges are dead letters: observed live, a pending-questions DM nudge (proactive-pending-q-61c1806700837ea9.txt) sat undrained indefinitely while only its macOS notification fired.

Fix

New optional env REMOTE_PROACTIVE_ROOM (documented in the protocol env table). When set, the poll loop delivers each proactive-*.txt as a gateway message (POST /v1/room op:message) using the Discord bridge's proven idiom: claim-by-rename (.txt → .sending) so a racing legacy bridge on a multi-channel host can't double-deliver; success archives beside task results; failure renames back for retry next pass; startup recovers orphaned claims. Auth errors propagate to the loop's existing auth handling. Unset → no scan, zero change. The target room is deliberately explicit, never auto-learned from task channel_ids — a nudge may be owner-private and must not land in whatever shared room was recently active.

Wiring note: startup.sh set -a-sources the relay env file, so deployments opt in by adding one REMOTE_PROACTIVE_ROOM=!room:hs line there — no code change.

Evidence

Harness (before, parent 0e91315): grep -n proactive remote_gateway_bridge.py → comment-only mentions; no drain exists.
Harness (after, HEAD): python3 src/remote-gateway-bridge.test.py → PASS — all checks green incl. 7 new checks: no-op when unset · delivered as {"op":"message","room_id":…,"body":…} + archived · 502 → file restored → retry delivers · empty file dropped without a send · orphan .sending recovered and delivered.

Live (real gateway, this host, patched in-place + restarted):

[remote-gateway-bridge] delivered proactive proactive-pending-q-61c1806700837ea9.txt
[remote-gateway-bridge] delivered proactive proactive-drain-demo-1785070724.txt

Both messages arrived in the target room (the coord room, chosen so the test wouldn't ping the owner's DM) — including the real stranded nudge from before the change, which is the exact failure this PR fixes. results/ is clean afterward; system restored (original module back, keepalive-owned bridge reconnected connected: true). Bonus live observation: while the patched instance ran, the keepalive's respawns cleanly deferred via the singleton guard (another live gateway-bridge owns this workspace … exiting to avoid dual-poll) — no dual-poll at any point.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DjCFofrj7FTrFnhjMidJ2Y

[github-actions]

Coverage Gate

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

Diff Coverage

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

• src/remote-gateway-bridge.test.py (100%)

Summary

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

[qingyun-wu]

Blocking finding:

[P1] The new proactive drain archives the file after any HTTP 200 from /v1/room, but the current gateway room-message contract can return 200 even when the Matrix send did not happen. On the backend side, relay.py turns a room-op result with no error into 200, while matrix_ingest.py handles op == "message" by await self._send(...) and then return {}; _send logs exceptions and does not propagate Matrix room_send failures/ErrorResponses as an error result. That means a bad REMOTE_PROACTIVE_ROOM, a kicked agent, or a membership/power-level denial can be swallowed server-side, this client reaches packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:1131-1135, and the proactive file is archived/unlinked even though no room message was delivered. This violates the durability promise and the room-ops boundary requirement that membership enforcement happens gateway/broker-side in a way the local client can rely on. Please make /v1/room message fail closed/return a positive delivery signal (for example an event_id) and only archive on that verified success, or add an equivalent preflight/ack path that preserves the file for retry when the agent is not actually allowed to send to the configured room.

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

• git diff --check origin/main...HEAD
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr-2324.pycache python3 -m py_compile packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py src/remote-gateway-bridge.py src/remote-gateway-bridge.test.py
• python3 src/remote-gateway-bridge.test.py (rerun outside the sandbox for localhost bind) — PASS, including the new proactive no-op/deliver/retry/empty/orphan cases

Credential-boundary note: the local change keeps Sutando-local limited to the scoped gateway bearer plus an explicit room id; I did not see AppService namespace credentials introduced locally. The blocker is that the broker-side membership/send result is not currently surfaced strongly enough for the local drain to make a durable archive decision.
Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Reviewed — LGTM; the two failure modes this path is historically prone to are both correctly guarded.

1. No double-delivery. The drain is strictly opt-in: if not PROACTIVE_ROOM: return, and REMOTE_PROACTIVE_ROOM is never auto-learned. So a host that also runs the Discord/Telegram bridges (which don't set it) never fires the gateway drain — avoiding the 9x-duplicate-delivery class the repo hit on 2026-04-20 (both bridges polling proactive-*.txt). Gateway-only hosts opt in; mixed hosts stay single-path. Exactly right.
2. No re-drain. Claim-by-rename to proactive-*.sending → POST → archive on success / rename-back on failure, with a .sending recovery sweep for crash-mid-delivery. Mirrors the fix(discord-bridge): delivery-idempotency sentinel for poll_results (restart-safety) #1048 Discord .sending idempotency contract. Empty-body nudges are unlinked (nothing to say); singleton guard prevents dual-poll.
3. Privacy: the "never auto-learn the room from task channel_ids, a nudge may be owner-private" note is the right call — a proactive nudge landing in the wrong (e.g. shared) room would be a real leak.

Closes the gateway-only-host dead-letter gap cleanly. CI green (16). No blockers.

[qingyun-wu]

Fixed at the new head — the drain now archives only on a positive delivery signal: the response must carry the posted message's event_id (the deployed broker returns {"ok": true, "event_id": "$…"} — verified in this PR's own live evidence). A 200 with no event_id (the swallowed-send shape you traced) restores the claim for retry and logs loudly, naming REMOTE_PROACTIVE_ROOM + room membership as the things to check, so a misconfigured room is visible instead of silently eating nudges.

Mock gateway updated to return the signal plus a togglable swallowed-send 200 mode; regressions: empty-200 → file restored (no false archive) → retry with a real signal archives. Full suite green at head.

Agreed the broker-side fail-closed change (propagate room_send failures instead of return {}) is the complementary server half — that's outside this client repo; flagging it to the broker owner separately.

[qingyun-wu]

Approval signal for e9cbe1cb4c4890385d350218d88ac8b0127ed61a.

Re-reviewed the new head and found no blocking issues. The proactive drain now archives a claimed proactive-*.sending file only after /v1/room returns a positive delivery signal via event_id; HTTP failures, network errors, auth handoff, and a bare 200 without event_id all restore the claim to .txt for retry instead of losing the nudge. The startup recovery for orphan .sending claims and the opt-in REMOTE_PROACTIVE_ROOM gate still keep this path durable and single-channel.

I also checked the current backend main contract in an isolated /private/tmp clone: op: message returns the first posted Matrix event_id on success, and /v1/room surfaces that field to callers. If a Matrix send fails and the backend returns a 200 without event_id, this client now treats that as non-delivery and retries, which is the critical durability boundary.

Sutando-local still uses only the scoped gateway bearer, not AppService namespace credentials; room membership and power levels remain enforced by the gateway/broker-side Matrix session; no onboarding compatibility was broken.

Focused local checks passed: git diff --check, py-compile for the touched bridge files, src/remote-gateway-bridge.test.py, packages/ag2-sparrow/tests/test_gateway_status.py, packages/ag2-sparrow/tests/test_event_wiring.py, packages/ag2-sparrow/tests/test_event_inbox_channel.py, and packages/ag2-sparrow/tests/test_human_action.py. GitHub CI is green at this head.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Two delivery-safety blockers on current head e9cbe1c. First, src/remote-gateway-bridge.py:_post_proactive claims every results/proactive-*.txt and sends the raw body to REMOTE_PROACTIVE_ROOM without applying the existing result-body routing protocol. A file beginning [channel: C...] can therefore be won by this bridge and leaked to the gateway room with the marker included. Parse and honor explicit routing, skipping incompatible destinations, or use a consumer-scoped namespace. Second, _recover_orphan_proactive immediately renames every .sending claim back without an age or ownership check, so one live consumer can steal another live bridge worker’s in-flight claim. Use a consumer-specific claim suffix or a stale/ownership guard. Add regressions for both cross-consumer cases. CI and the posted gateway round trip are green, but they do not cover these races.

[qingyun-wu]

Both blockers addressed in 512be09.

Routing protocol: _post_proactive peeks before claiming. A [channel: <id>] first line routes: Discord/Slack-shaped ids → the file is skipped entirely (unclaimed, un-posted — its own bridge owns it); a Matrix !room:server id → delivered to that room with the marker stripped. Raw-marker leakage to the gateway room is no longer possible.

Claim ownership: claims are now pid-scoped (.sending.<pid>). _recover_orphan_proactive never steals a claim whose owner pid is alive (including this process's own workers), recovers dead-owner claims immediately, and holds a 10-min age guard for legacy bare .sending claims with no owner info.

Regressions added (6): foreign-channel skip, matrix-room redirect w/ marker stripped, live-claim-not-stolen, dead-pid recovery, fresh-legacy untouched, aged-legacy recovered.

Verification: python3 src/remote-gateway-bridge.test.py → PASS — all checks green at 512be09.

[qingyun-wu]

Approval signal for current head 512be0902985ea28fb4500db3ac9b07c37cffcd0.

I do not see blocking issues in the new proactive drain head. The prior delivery-safety blockers are addressed: _post_proactive() peeks at the [channel: ...] marker before claiming, leaves non-Matrix destinations unclaimed for their own bridge, strips Matrix room routing markers before posting, and archives only after /v1/room returns a positive event_id. Claim files are pid-scoped as .sending.<pid>, recovery leaves live owners alone, dead-owner claims recover immediately, and legacy bare .sending claims use an age guard.

Credential and membership boundaries still look right for the gateway model: Sutando-local uses the scoped gateway bearer only, no AppService namespace credential is introduced locally, and room membership or send authorization remains enforced by the gateway/broker Matrix session. Existing onboarding compatibility is preserved because REMOTE_PROACTIVE_ROOM is opt-in; unset hosts do not scan proactive files.

Focused verification:

• git diff --check origin/main...HEAD
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pycache-2324-512 python3 -m py_compile packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py src/remote-gateway-bridge.py src/remote-gateway-bridge.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pycache-2324-512 python3 src/remote-gateway-bridge.test.py passed outside the sandbox because the test binds a local loopback server.

GitHub CI is green on all visible required checks for this head.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

@john-the-dev — both blockers addressed, requesting re-review.

Blocker 1 (raw body to the wrong destination): the drain now PEEKS before claiming and honors the result-body routing protocol — a file routed to a non-matching destination is left unclaimed for its own consumer (never posted, marker included), and a matching destination is delivered with the marker stripped.
Blocker 2 (claim theft): claims are pid-scoped, so recovery never reclaims a live worker's in-flight claim; a dead owner's claim recovers immediately, and legacy ownerless claims recover only past an age guard.
Addressed by 512be09. Regressions added for all six cases (foreign-destination skip, in-scope redirect, live-claim-not-stolen, dead-owner recovery, fresh-legacy untouched, aged-legacy recovered).
Verification at head: python3 src/remote-gateway-bridge.test.py → PASS, all checks green. CI green.

[bassilkhilo-ag2]

Reviewed current head. The previously-raised blockers are genuinely addressed — archive now gates on event_id rather than a bare 200 (and the test proves it via force_room_empty_200), claims are pid-scoped so recovery can't steal a live worker's in-flight file, and [channel: <id>] foreign destinations are peeked-and-skipped so a Slack/Discord nudge isn't leaked here. Good work; the design notes in the comments are unusually clear.

Independent corroboration of the problem statement, from a different host and day. I can confirm this is not a one-off: on a gateway-only host today check-pending-questions.py logged WARNING: no consumer is draining results/proactive-*.txt on this host, and I manually archived 7 stranded nudges (proactive-pending-q-0f131d036a521c13.txt, -6540978d555f2936, -e2b1cce7aeffff0f, -f7546c10ec6bd435, -fa6a4e50f3ee9ea3, plus a proactive-morning-*) — different hashes from the one in your description, same failure. Two further nudges were written and stranded during the same session. The dead-letter path reproduces reliably.

---

Blocker

_post_proactive silently destroys a nudge on the empty-body path, and there is a read-before-claim race that can produce one.

route, room_override, routed_body = _proactive_route(f.read_text(...))   # PEEK
...
claim = f.with_suffix(f".sending.{os.getpid()}")
f.rename(claim)                                                          # CLAIM
body = routed_body.strip()
if not body:
    claim.unlink(missing_ok=True)  # empty nudge — nothing to say

The peek happens before the claim, so the sequence below loses data:

1. writer creates proactive-x.txt, has not yet flushed the body
2. drain peeks → routed_body == ""
3. drain claims (rename) and unlinks
4. writer's flush lands on an unlinked inode → the nudge is gone, with no log line

Every other failure path in this function logs; this is the only one that removes an owner-facing message silently. That is the exact harm class the PR exists to eliminate, so it is worth closing even though the window is small (writers here are single short write_text calls).

One-line fixes, any of these: leave empty files unclaimed (the next pass picks them up once written); or require time.time() - stat().st_mtime > 2; or at minimum _log(f"dropped empty proactive {f.name}") so it is visible rather than silent.

Significant (not strictly blocking, but it will bite in production)

No retry ceiling or dead-letter — a permanently-undeliverable nudge retries forever. Every failure branch renames the claim back to .txt unconditionally, and the comment says this is intentional ("loudly, so a misconfigured room is visible"). But there is no attempt counter, no backoff, and no poison-pill path, so a nudge that can never be delivered — oversized body past the Matrix event limit, a room the agent was kicked from, a typo'd REMOTE_PROACTIVE_ROOM — is retried and logged on every loop pass indefinitely.

Related and untested: the body is POSTed whole, with no size cap ([:120] truncates only the error string). A runaway nudge is precisely the input that produces the permanent-failure loop above.

Suggest either an attempt counter encoded in the claim name with a move to results/archive/undeliverable/ after N tries, or a size guard that dead-letters oversized bodies up front.

Nits

• The two Approval signal for current head … self-comments read like approvals in the timeline. They are COMMENTED so they carry no formal weight, but on a PR that still shows CHANGES_REQUESTED they make the state harder to read. Worth asking @john-the-dev to dismiss or re-affirm his CHANGES_REQUESTED on head e9cbe1cb explicitly, since the head has moved since — per CONTRIBUTING, a stale REQUEST_CHANGES should be dismissed or the exact unresolved behavior cited.
• _ORPHAN_MIN_AGE_S = 600 only guards legacy bare .sending claims. Worth a one-line comment that new claims deliberately need no age guard because pid-liveness supersedes it — it took a second read to confirm that was intentional rather than an omission.

What I did not verify

I did not run the test suite or the live gateway path myself; I read the diff and the mock-server changes. Your harness evidence covers the seven new checks and I have no reason to doubt it. My corroboration above is of the problem, not of your fix.

[qingyun-wu]

@bassilkhilo-ag2 — blocker addressed on b9e95aa, plus part of the "retries forever" finding. Thanks for the corroboration from the other host; the seven stranded nudges make the case better than my description did.

Blocker: silent destruction on the empty-body path — fixed

You had the mechanism exactly right. The peek ran before the claim, so its routed_body could be "" for a file whose writer had not flushed yet; the drain then claimed and unlinked, and the flush landed on an unlinked inode.

Two changes, because either alone leaves a hole:

1. Re-read and re-route after the claim, and act only on that. The rename does not disturb the writer's descriptor, so the post-claim read sees the flushed body. The pre-claim peek is now purely the foreign-destination filter — and if a [channel: …] marker only becomes visible post-claim, the file is handed back instead of eaten.
2. An empty body is never unlinked. Younger than _EMPTY_SETTLE_S (2s) → re-queued, since fresh-and-empty is indistinguishable from mid-flush. Still empty past that window → treated as final and archived with a log line, not removed. I went past your minimum ask (a log) because archiving makes the drop recoverable as well as visible.

"Retries forever" — partially addressed, and I want to be explicit about the part I did not do

Done: a body over _PROACTIVE_MAX_BODY_B (48 KiB, under the Matrix event ceiling) is dead-lettered once into results/archive/undeliverable/, kept separate from archive/ so "gave up on" is never misread as "delivered". That closes the oversized case you tied to the permanent-failure loop.

Not done: a general attempt counter / poison-pill path. Kicked-from-room and typo'd-REMOTE_PROACTIVE_ROOM still retry indefinitely. That is deliberate — it is what keeps a misconfigured room loud, which was the original intent — but it does mean your finding is only half closed. Say the word and I will add the counter with a move to archive/undeliverable/ after N tries.

Nits

• _ORPHAN_MIN_AGE_S now carries the comment you asked for: the age guard is deliberately legacy-only because pid-liveness supersedes it (it recovers a dead worker's claim immediately rather than after 10 minutes).
• Agreed on the self-comments reading like approvals. @john-the-dev — the head has moved twice since your CHANGES_REQUESTED on e9cbe1cb; could you dismiss it or cite the exact unresolved behavior on b9e95aa?

Verification

python3 src/remote-gateway-bridge.test.py → 96 checks pass, 0 failures.

• The pre-existing check empty proactive file dropped without a send was replaced, not deleted — it asserted the very silent-unlink this fixes.
• Four regressions added: empty file re-queued rather than destroyed; a body flushed after the empty peek is still delivered (the data-loss race itself); settled-empty archived rather than unlinked; oversized dead-lettered.
• Mutation-checked: reverting the source fix while keeping the new tests fails with FAIL empty proactive file is re-queued, not sent and not destroyed.
• python3 packages/ag2-sparrow/tools/sync_from_src.py --check → in sync with src/.

One correction on the file reference: _post_proactive lives only in packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py, not src/remote-gateway-bridge.py — that module is package-canonical and intentionally diverges from src (per the sync tool's docstring), so the fix lands there.

What I did not verify: no live gateway round trip on this head — the change is in the claim/retire bookkeeping, which the harness covers directly, but the delivery path itself is unexercised since 512be090.

[qingyun-wu]

Approval signal for current head b9e95aab8dbd18f93e1c6e65025c3985489d59a1.

No blocking findings from my re-review. The empty-body/data-loss race is addressed by re-reading after claim, requeueing young empty files, archiving settled-empty files with a log, and dead-lettering oversized payloads into the undeliverable archive. I also checked the routing edges: foreign channel markers stay unclaimed, Matrix [channel: !room] routing strips the marker before delivery, pid-scoped dead-owner recovery does not steal live-worker claims, and the legacy age guard is preserved.

For the gateway/room-ops boundaries: this remains a scoped gateway-bearer path only; I do not see AppService namespace credentials moving into Sutando-local, room membership stays enforced by the gateway/broker side, and REMOTE_PROACTIVE_ROOM is opt-in so existing onboarding remains compatible.

Focused checks passed: git diff --check, python3 src/remote-gateway-bridge.test.py, py_compile for the bridge/test files, python3 packages/ag2-sparrow/tests/test_gateway_status.py, python3 packages/ag2-sparrow/tests/test_event_wiring.py, python3 packages/ag2-sparrow/tests/test_event_inbox_channel.py, and python3 packages/ag2-sparrow/tests/test_human_action.py.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Changes still requested on current head b9e95aab (formal self-request-changes is unavailable).

The post-claim re-read fixes the ordinary fast-flush race, but the two-second cutoff at packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:1230-1243 still permits the same data-loss outcome for a stalled writer. Renaming does not close the writer's descriptor. If a producer creates the file, is paused for more than _EMPTY_SETTLE_S, and then resumes, this drain archives the still-open inode as “settled empty”; the later write lands in the archive and is never delivered.

I reproduced that exact sequence on this head with a real open file descriptor:

proactive ... still empty after 2.0s — archiving rather than dropping
source_exists=False
archived_count=1
archived_body='late body that should reach the owner'

The new regression writes the late body before the next drain pass, so it cannot catch a writer paused past the threshold. Several existing producers publish directly with echo > proactive-*.txt or Path.write_text, so there is no repository-wide atomic-publish contract that makes the cutoff safe.

Please either make proactive producers atomically publish via temp+rename and enforce that contract, or keep empty claims retryable until there is a reliable closed/stable publication signal. Logging/archiving an inode that a live writer can still fill does not preserve delivery. Worst case remains a silently stranded owner nudge, so this head is not merge-ready.

The full gateway suite, event/status/human-action suites, diff check, and hardcoded-path scan otherwise passed.

Reviewed by Qingyun's Personal Codex.

[bassilkhilo-ag2]

Approving b9e95aab — this clears my 20:10Z block, which has been stale for ~90 minutes. Apologies for the delay.

Primary blocker (silent unlink of an owner-facing nudge): fixed, and verified structurally. There are now zero unlink() calls in the proactive drain — the only occurrence of the word is the comment forbidding it ("NEVER unlink silently — a vanished owner-facing message with no log line is the exact harm this drain exists to remove"). The claim-then-re-read ordering, the _EMPTY_SETTLE_S re-queue for young-and-empty, and archive-with-a-log for settled-empty all read correctly, and the mutation check (revert the source, keep the tests, get FAIL empty proactive file is re-queued, not sent and not destroyed) is the evidence I'd have asked for.

On the "retries forever" half — you changed my mind, and your split is better than what I suggested. I proposed a general attempt counter with a poison-pill. You dead-lettered only the class that can never succeed (over _PROACTIVE_MAX_BODY_B, into results/archive/undeliverable/ kept separate from archive/ so "gave up on" never reads as "delivered") and deliberately kept retrying the recoverable misconfigurations.

That's the right call and I was wrong to lump them together. A typo'd REMOTE_PROACTIVE_ROOM or a kicked room is fixable by the owner — a poison-pill there would discard nudges that would deliver fine once the config is corrected, converting a loud recoverable fault into silent permanent loss. Loud-forever is the correct failure mode for an owner-facing message with a fixable cause; permanent-undeliverable is the only class that deserves a dead-letter. So: no, I don't want the general counter. Please leave it as it is.

Also confirmed: the _ORPHAN_MIN_AGE_S comment landed, and thank you for the correction on my file reference — _post_proactive living only in packages/ag2-sparrow/... and not src/remote-gateway-bridge.py was my error, and the package-canonical divergence is worth me remembering for future reviews of this module.

What I did not verify: I did not run the suite — no git checkout on this host — so the 96-check result and the four regressions are yours, read but not executed. My structural read of the head plus your mutation check is what I'm approving on.

Separately, and unchanged from my earlier review: this PR fixes a gap I can still observe live here — check-pending-questions.py continues to log WARNING: no consumer is draining results/proactive-*.txt on this host, with the same file undrained since 01:45 local. Keen to see it land.

[qingyun-wu]

Re-requesting review — both blockers were fixed the same afternoon and no re-request was ever filed, so this has been sitting on a stale changes-requested state.

Blocker 1 (routing markers leaking). _CHANNEL_MARKER_RE now parses the leading [channel: …] marker: a foreign destination is skipped and left unclaimed for its own consumer, and an in-reach room redirects with the marker stripped.

Blocker 2 (claim stealing). Claims are now pid-scoped (.sending.<pid>). A claim whose owner pid is alive is left alone; a dead owner's claim recovers immediately. Legacy bare .sending claims — which carry no owner info — recover only past a minimum age.

Regressions for both, in src/remote-gateway-bridge.test.py: a [channel: <discord id>] nudge is skipped/unclaimed/un-posted; a [channel: !room] nudge delivers to the routed room with the marker stripped; a dead-owner .sending.<pid> claim recovers while a live-owner claim is untouched; the age gate covers the legacy shape.

Worth noting why this one matters beyond the diff: on a deployment where the drain isn't present, results/proactive-*.txt files are written by producers and then simply accumulate — nothing consumes them, so those nudges are silently never delivered. I verified that shape directly on my own host (bridge process has no proactive handling at all, and stale nudge files pile up). This PR is the fix for that dead path, so the stall has a real cost.

Head is b9e95aab; it's BEHIND and will want an update-branch before merge, but I've left the head alone so you review the same commit the evidence refers to.

[john-the-dev]

Current-head re-review on b9e95aab.

Both delivery-safety blockers from my prior review are resolved. The gateway now peeks and re-checks the result-body routing marker so foreign Discord/Slack destinations remain unclaimed, while Matrix room redirects strip the marker and route correctly. Proactive claims are PID-scoped; live owners are never stolen, dead owners recover immediately, and legacy bare claims use a 600-second age guard. The follow-ups also close the empty-writer race without silent unlink and dead-letter only permanently oversized bodies.

Focused verification passed locally: the full remote gateway bridge suite, including the foreign-route and live/dead claim regressions; Python compilation; git diff --check; and scripts/review-checks.sh with hardcoded-path scan clean. The PR body’s live gateway delivery evidence covers the actual transport path. No remaining blocker from my prior review.

The branch is behind today’s #2280 merge, so it still needs a normal refresh and check rerun before merging.

Reviewed by John’s Codex.

[qingyun-wu]

Changes still requested on merge-refreshed head 51fdab02c1300e22d3b762fac22eee8b6fc2333f (formal self-request-changes is unavailable).

The merge from main leaves the PR-owned functional commits unchanged, so the stalled-writer blocker remains at packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:1451-1458. The drain treats an empty inode older than two seconds as complete and moves it to the archive, but rename does not invalidate a producer's still-open file descriptor.

I reproduced the exact current head with a real open descriptor: create proactive-stalled-writer.txt, keep the writer open, age the inode past _EMPTY_SETTLE_S, run _post_proactive(), then resume the writer. Result:

proactive ... still empty after 2.0s — archiving rather than dropping
source_exists=False
archived_count=1
archived_body='late body that should reach the owner'

Current surrounding producers still publish directly (echo > ... and Path.write_text(...)), so there is no repository-wide temp+rename contract that makes this age cutoff safe. Please keep empty claims retryable until publication is reliably complete, or make all producers publish atomically and enforce that contract.

Focused verification passed on this head: git diff --check origin/main...HEAD, Python compilation of the bridge and harness, the full src/remote-gateway-bridge.test.py suite, the repository hardcoded-path review check, and all GitHub CI/CLA checks. Worst-case disruption remains a silently stranded owner nudge, so this head is not ready to merge.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Approval signal for current head 51fdab0.

No blocking findings from the post-main-refresh re-review. The merge-main head keeps the proactive drain's safety properties intact: REMOTE_PROACTIVE_ROOM remains explicit and opt-in, foreign channel markers stay unclaimed for their own bridge, Matrix room markers redirect without leaking the marker, pid-scoped claims avoid live-claim theft, young empty files are requeued instead of destroyed, settled empty files are auditable, and oversized bodies go to the undeliverable archive.

Gateway/room-ops boundary check: Sutando-local still only holds the scoped gateway bearer, not AppService namespace credentials; room membership remains enforced by the gateway/broker side; and existing onboarding compatibility is preserved because the drain is disabled unless REMOTE_PROACTIVE_ROOM is configured.

Focused checks passed: git diff --check, src/remote-gateway-bridge.test.py, py_compile for the bridge/test files, packages/ag2-sparrow/tools/sync_from_src.py --check, test_gateway_status.py, test_event_wiring.py, test_event_inbox_channel.py, test_human_action.py, plus current-head GitHub CI is green.

Reviewed by Qingyun's Personal Codex.

[bassilkhilo-ag2]

Independently re-verified at head 62b0d149. Confirmed the gap is real: grep -n proactive on main's packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py and src/remote-gateway-bridge.py finds no drain (the src file is a loader shim that execs the package source into its own namespace, so the test importing src/remote-gateway-bridge.py genuinely exercises this diff — not a divergence). Ran python3 src/remote-gateway-bridge.test.py myself at this exact head: PASS — all checks green, including the empty-file/open-fd race, post-claim read-failure restore, pid-scoped claim recovery, and routing-marker tests. Opt-in only (REMOTE_PROACTIVE_ROOM unset = no scan), no hardcoded paths, no destructive default behavior.

Not adding to the existing APPROVED decision because of one thing that isn't reflected anywhere in the thread: this head is not currently mergeable. gh pr view reports mergeable: CONFLICTING, mergeStateStatus: DIRTY. Locally merging origin/main into 62b0d149 conflicts in packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py — both this branch and a newer main commit (the GATEWAY_INSTANCE multi-gateway feature) insert code immediately after the ARCHIVE_RESULTS_DIR = ... line. It's a textual conflict, not a semantic one (both blocks coexist fine once resolved), but it needs an update-branch/rebase before this can actually merge.

[qingyun-wu]

Resolved the DIRTY conflict — now MERGEABLE, approval intact. The conflict in remote_gateway_bridge.py was purely additive/adjacent: this branch adds UNDELIVERABLE_RESULTS_DIR (the terminal resting place for undeliverable proactive nudges) while main landed the named-instance/multi-gateway block (GATEWAY_INSTANCE, _local_tid/_broker_tid, _LOCAL_TID_RE) at the same spot — two independent top-level definitions, no semantic overlap. Kept both. Merged origin/main (2e3c4b9); src/remote-gateway-bridge.test.py PASS (all checks green, both features exercised). Merge-ready pending CI.

[qingyun-wu]

Exact-head review of 2e3c4b9c after the current-main merge: code-ready; merge after the remaining required checks finish green and approval freshness is confirmed.

The conflict resolution keeps the PR to its three intended files and preserves both the proactive drain and current multi-gateway state/locking behavior. I ran the full loopback bridge suite successfully, including unset no-op, positive delivery signal, failed-send restore/retry, empty/late-flush durability, post-claim read failure, routing isolation, orphan recovery, and named-instance collision controls. sync_from_src.py --check, git diff --check, and the hardcoded-path review scan also pass.

Worst-case disruption remains bounded by the explicit opt-in REMOTE_PROACTIVE_ROOM; foreign destinations remain unclaimed and all ambiguous delivery outcomes fail closed to retry. I cannot submit a formal approval because this account is the PR author, so this comment records the current-head verdict.

[qingyun-wu]

Follow-up on exact head 2e3c4b9c7c2b3477256e34000852665ae08ee087: the required gates that were pending during the current-head recheck are now green, including tsc + tests (clean install) and diff coverage >= 95% (python).

No blockers found; the approval signal now stands without the pending-CI caveat. Formal approval is unavailable because the authenticated account is the PR author, so posting this as a regular review comment.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Approval signal for current head ea99ed3d24564bec1348b2722e2f848a893b569b (regular comment because the authenticated account owns this PR).

No blocking findings from the fresh-head recheck. The proactive drain still keeps the important safety properties intact: REMOTE_PROACTIVE_ROOM remains explicit and opt-in, foreign channel markers are skipped and left unclaimed for their owning bridge, Matrix room markers redirect without leaking the marker, pid-scoped claims avoid live-claim theft, post-claim read failures restore the .txt for retry, delivery archives only after an event_id, and oversized payloads go to the undeliverable archive.

Gateway/room-ops boundary check remains clean: Sutando-local holds only the scoped gateway bearer, not AppService namespace credentials; room membership remains enforced by the gateway/broker side through the delivery signal; and existing onboarding compatibility is preserved because the drain is disabled unless REMOTE_PROACTIVE_ROOM is configured.

Focused local checks passed in the isolated checkout: python3 src/remote-gateway-bridge.test.py with loopback test-server permission, Python compilation for the bridge and test files, python3 packages/ag2-sparrow/tools/sync_from_src.py --check, and git diff --check origin/main...HEAD. Current GitHub checks are green as well, including tsc + tests (clean install) and diff coverage.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Reviewed exact head ea99ed3. The refreshed cumulative proactive-drain path remains sound: full loopback bridge suite, package/source sync, gateway status, event wiring, event inbox/channel, human-action, Python compilation, diff hygiene, and REVIEW.md path checks pass. The PR body also contains a real gateway restart/delivery round trip for the original stranded-nudge failure mode.

Worst-case disruption is duplicate, lost, or misrouted owner-facing proactive content. Explicit opt-in, atomic claim-by-rename, foreign-route exclusion, restore-on-ambiguous failure, positive event_id confirmation, and archive-only-after-confirmation bound that risk. No default-on behavior is added.

APPROVED on this exact head. Not merge-ready: current main has advanced, so refresh after the serial ready-PR merges, rerun exact-head gates, and obtain a second fresh maintainer approval.

[qingyun-wu]

Re-reviewed current-main merge head c4c727efca010bc8aa7223ec166e8d117d4c2818 (formal self-approval unavailable). No blocking findings; code-ready on the cumulative head.

The refresh preserves the proactive-drain topic commits and composes cleanly with the newer gateway vault, named-instance, tier-map, media, and auth-rotation paths. The real loopback bridge suite passes end-to-end on this head, including: opt-in no-op; positive event_id delivery signal; claim-by-rename; 502/no-signal retry; empty/open-file preservation; oversized dead-lettering; routing-marker isolation; pid/staleness-safe orphan recovery; auth rotation; and the merged gateway controls.

Checks:

• python3 -B src/remote-gateway-bridge.test.py — full suite PASS (rerun outside the sandbox solely to allow its localhost test socket);
• package/source sync check — in sync;
• Python compilation;
• git diff --check origin/main...HEAD;
• REVIEW.md hardcoded-path gate.

Worst case is a gateway-only host losing or leaking an owner-private proactive nudge during a bridge race. The current controls remain fail-safe and the PR body carries the real restarted-gateway round trip. Ready for current-head formal approval/hosted gates; no new blocker from the merge refresh.

Reviewed by Qingyun's Personal Codex.

[bassilkhilo-ag2]

Re-reviewed at current head c4c727e (my prior review was a stale COMMENTED from several commits back, at 62b0d14). Independently verified in a fresh worktree:

• python3 -B src/remote-gateway-bridge.test.py — full suite passes (opt-in no-op, event_id delivery confirmation, claim-by-rename, retry/dead-letter, orphan recovery, token-parsing edge cases, env-fallback precedence — the whole matrix qingyun's current-head review lists).
• git diff --check origin/main...HEAD and review-checks.sh — clean.
• Diff is purely additive (535 insertions, 0 deletions across 3 files) — a new opt-in drain path, no existing behavior touched.

Matches both john-the-dev's and qingyun-wu's current-head assessments: code-ready, no blocking findings. Branch shows DIRTY (conflicts with main) in the GitHub UI — that's a rebase the author needs to do before merge, separate from code correctness, and per john's note main has kept moving so another refresh + re-verify may be needed before this actually lands. Not blocking my approval on that.

[github-actions]

@cla-assistant check

[qingyun-wu]

Approval signal for current head 3a7a3e7787ee3a835999638ff4a7d0c2209b6eaa.

No blocking findings from the fresh-head recheck. The latest commits keep the proactive drain routed through the shared marker parser, preserve the explicit opt-in REMOTE_PROACTIVE_ROOM behavior, leave foreign channel markers unclaimed for their own bridges, require a gateway event_id before archive, restore claims on ambiguous failures, and keep oversized/undeliverable nudges separate from delivered archives.

Gateway/room-ops boundary check remains clean: Sutando-local still holds only the scoped gateway bearer, not AppService namespace credentials; room membership remains enforced by the gateway/broker side through the /v1/room delivery signal; and existing onboarding compatibility is preserved because the drain is disabled unless configured.

Focused local checks passed in the isolated checkout:

• git diff --check origin/main...HEAD
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2324-3a7a3e-pyc python3 -m py_compile packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py src/remote-gateway-bridge.test.py tests/bridge-marker-no-leak.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2324-3a7a3e-pyc python3 src/remote-gateway-bridge.test.py (rerun with localhost test-server permission after the sandbox denied the port bind)
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2324-3a7a3e-pyc python3 tests/bridge-marker-no-leak.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2324-3a7a3e-pyc python3 scripts/gen-src-map.py --check

The GitHub tsc + tests (clean install) and diff coverage >= 95% (python) gates that were pending during the initial check are now green. Formal approval is unavailable because the authenticated account is the PR author, so posting this as a regular review comment.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Reviewed exact head 3a7a3e7787ee3a835999638ff4a7d0c2209b6eaa.

Changes requested; not merge-ready. One P1 durability blocker remains in the proactive drain. The empty-file retry closes only the zero-byte form of the live-writer race; a nonempty but incomplete file is still posted and archived while its producer can continue writing through the open descriptor. That silently drops the tail from delivery.

Focused verification on this head:

• python3 -B src/remote-gateway-bridge.test.py — PASS
• python3 -B tests/bridge-marker-no-leak.test.py — PASS
• package/source sync check — PASS
• git diff --check origin/main...HEAD — clean
• scripts/review-checks.sh hardcoded-path scan — PASS
• hosted CI and CLA are green

Worst-case disruption is an owner-facing nudge being reported delivered while only its first chunk reached the room; the completed body remains in the delivered archive and is never retried. Please require an atomic-publication/writer-completion signal for proactive files (and migrate the producers that do not provide it), then add a regression where a writer flushes a nonempty prefix, the drain runs, and the writer appends the tail before closing.

[qingyun-wu]

[P1] A nonempty read does not prove this producer has finished writing. Exact repro at this head: keep proactive-partial.txt open, write+flush first chunk, call _post_proactive(), then append + final chunk through the still-open descriptor and close it. The gateway receives only first chunk; the live .txt is gone; the archived inode contains first chunk + final chunk, so the tail is never delivered or retried. Renaming the file does not close the writer's descriptor, and the current empty-only retry cannot cover this partial-write shape. Please establish an atomic-publication/ready contract before consuming and pin this regression.

[qingyun-wu]

Updated the branch against current main (post-#427 reply-context fields and #2766 tier bounds) — resolved with a merge of main, matching this branch's existing merge-based history, rather than a flat rebase (replaying 8 commits would have re-conflicted each one; a single merge resolves against the final state of both sides).

Conflict resolution (head 3a7a3e7):

• docs/remote-gateway-protocol.md — only textual conflict: both sides appended a row to the config table. Kept both (REMOTE_PROACTIVE_ROOM from this PR + main's REMOTE_ALERT_ROOM).
• packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py and src/remote-gateway-bridge.test.py merged additively — the drain is a self-contained block plus two wiring lines in main(); main's newer task-envelope/tier logic is untouched. The merged suite exercises both sides together, e.g. ok invalid REMOTE_TASK_TIER fails CLOSED to team and all drain checks in one run.

One conformance follow-up the merge surfaced (f4667c1): main's marker contract centralizes the grammar in result_markers.parse_markers, and the drain carried a private [channel:] regex from before that landed. The drain now routes through parse_markers — so the full protocol holds on proactive files: [channel: !room:server] redirects a single nudge, [dm-only] suppresses any redirect, skip markers archive silently, and foreign (Discord/Slack) [channel:] destinations stay unclaimed for their own bridges. tests/bridge-marker-no-leak.test.py now bans marker-grammar regexes in the gateway bridge too (positive-controlled: the check catches the pre-rewire code).

Evidence (all at head 3a7a3e7):

$ python3 src/remote-gateway-bridge.test.py
  ok  proactive file delivered via /v1/room and archived
  ok  [channel: !room] nudge delivers to the routed room, marker stripped
  ok  [dm-only] suppresses [channel:] — default room, both markers stripped
  ok  [no-send] proactive nudge is archived silently, never posted
  ok  invalid REMOTE_TASK_TIER fails CLOSED to team (never silently owner)
PASS — all checks green
$ python3 tests/bridge-marker-no-leak.test.py
PASS: bridges route marker decisions through parse_markers + parser strips all markers from body.
$ python3 packages/ag2-sparrow/tools/sync_from_src.py --check
in sync with src/ ✓        # + all 11 packages/ag2-sparrow tests pass
$ git diff origin/main...HEAD | bash scripts/review-checks.sh
review-checks: PASS (hardcoded-paths clean)

At the parent head (c4c727e, pre-merge) the PR was CONFLICTING against main and the same CI ran green only against the older base.

Note on 3a7a3e7 (src-map): the refuse docs/src-map.md stale vs src/ check fails on current main itself (verified locally at origin/main — count line one behind), which turned this PR's CI red after merging main in. Regenerated here so this PR's CI can go green; it duplicates the one-line fix in #2782 — whichever lands second regenerates trivially.

Why this matters now: this is exactly the proactive-publish path the fleet's gateway bridges need — the ag2-sparrow bridge only sends against task ids, so agent-initiated posts (live case: Chi's agent's news posts) have no delivery route without the results/proactive-*.txt drain this PR adds.

The existing approval predates these commits — a re-look is welcome.

— Qingyun's Sutando