fix(gateway): auth-rejection recovery — hot-reload a rotated token instead of crash-looping

[qingyun-wu]

Problem

On 401/403 the poll loop does sys.exit("FATAL: gateway auth rejected…"). Under a supervisor that blindly relaunches (the common desktop deployment), a revoked or expired bearer becomes a silent crash-loop — relaunch, one poll, FATAL, repeat every few seconds, forever — hammering the gateway edge and never surfacing to the user. It also makes server-side key-expiry policies unsafe to enable: the day a TTL lands, every expired client turns into one of these loops.

Fix

New optional env REMOTE_TASK_TOKEN_FILE — the durable token source (a dotenv-style file with a REMOTE_TASK_TOKEN= line, legacy AG2_REMOTE_TOKEN= honored, or the raw onboarding string alone on a line). On auth rejection the bridge now:

1. Re-reads the file. A different token there (the connect/onboarding flow re-ran while we were down/lagging) is swapped in live and polling resumes — no restart. _req reads the TOKEN global per call; the event channel + card poster now share one auth-header dict held by reference (they already build per-request copies), so the rotation propagates everywhere.
2. Otherwise waits in a slow re-check loop (REMOTE_AUTH_RECHECK_INTERVAL, default 30s), keeping the poller singleton heartbeated and writing a distinct gateway-status.json error (auth rejected HTTP N — waiting for re-connect) a supervisor can render. One live waiting process replaces the crash-loop; the moment the user re-runs the connect flow (which rewrites the file), the bridge picks it up.
3. Env unset → behavior unchanged: the historical FATAL exit is preserved bit-for-bit (verified in the suite).

startup.sh exports REMOTE_TASK_TOKEN_FILE alongside sourcing the relay env file (only when the file exists — env-only onboarding keeps the FATAL path).

Evidence

Harness (before, at parent commit fdfd7b4): the suite's section-5 401 check passes but the module has no recovery — grep -n "_recover_auth\|TOKEN_FILE" remote_gateway_bridge.py → no matches; the 401 branch is sys.exit only.

Harness (after, at HEAD): python3 src/remote-gateway-bridge.test.py → PASS — all checks green, including 9 new checks: dotenv/export/quote parsing, raw-string fallback, missing-file → no-rotation, no-TOKEN_FILE → False (FATAL preserved), unchanged-token → no-rotation, rotated combined url|secret swaps TOKEN + shared _AUTH_HEADERS, immediate-resume path, and the wait-loop picking up rotation after exactly one re-check. Affected package suites all pass: test_event_inbox_channel, test_human_action, test_event_wiring, test_gateway_status, human-action-bridge.test.py.

Live (real gateway, isolated dirs, bogus bearer — zero dual-poll risk by construction):

[remote-gateway-bridge] starting — gateway=https://chat.ag2.space/relay …
[remote-gateway-bridge] gateway auth rejected (HTTP 401) — waiting for token rotation in …/tok.env (re-check every 5s)
   # file rotated bogus-A → bogus-B while the SAME pid keeps running:
[remote-gateway-bridge] rotated token detected — resuming
[remote-gateway-bridge] gateway auth rejected (HTTP 401) — waiting for token rotation in …/tok.env (re-check every 5s)

gateway-status.json during the wait: {"connected": false, …, "error": "auth rejected HTTP 401 — waiting for re-connect", …}. One pid across the whole window — the crash-loop is gone; the rotation hot-swap fires against the real edge. (A valid-token resume can't be demonstrated without a second live bearer — the mock harness covers rotation→200; the happy-path code is untouched.)

Notes for review

• Single concern: recovery only. The token parse in _reload_rotated_token mirrors the module's current import-time "|" split on this head — it deliberately does not bundle the %7C-separator handling that's in flight in a separate PR; when that lands, the split should be factored into the shared helper (one-line follow-up in whichever lands second).
• _recover_auth exits if the poller singleton is definitively lost during the wait (same dual-poll protection as the main loop).
• The event channel reconnects with the rotated bearer on its next retry cycle by construction (per-request header copy off the shared dict).

🤖 Generated with Claude Code

https://claude.ai/code/session_01DjCFofrj7FTrFnhjMidJ2Y

[github-actions]

Coverage Gate

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

Diff Coverage

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

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

Summary

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

[qingyun-wu]

Blocking finding:

• [P1] The event channel can still die permanently during the auth-rejection recovery window. remote_gateway_bridge._recover_auth() now keeps the main poller alive while it waits for the token file to rotate and then mutates _AUTH_HEADERS in place, but EventChannel._consume_once() still treats 401/403 as fatal and returns False; EventChannel.run() then exits. In a real expired/revoked-token window, the SSE channel can reconnect with the stale bearer, get 401/403, and stop before the main poller hot-swaps the token. After _recover_auth() resumes, nothing restarts _EVENT_CHANNEL, so workspace events and human-action card delivery stay dead until the whole bridge process restarts. The shared header dict only helps if the channel is still running; the current test_channel_fatal_auth_stops behavior confirms it is not. Please either route the event channel through the same token-rotation wait/retry path or restart it after a successful reload.

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

• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr-2323.pycache python3 -m py_compile packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py packages/ag2-sparrow/ag2_sparrow/event_channel.py packages/ag2-sparrow/ag2_sparrow/human_action.py
• python3 src/remote-gateway-bridge.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr-2323.pycache python3 packages/ag2-sparrow/tests/test_event_inbox_channel.py
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr-2323.pycache python3 packages/ag2-sparrow/tests/test_human_action.py
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr-2323.pycache python3 packages/ag2-sparrow/tests/test_event_wiring.py
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr-2323.pycache python3 packages/ag2-sparrow/tests/test_gateway_status.py
• PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr-2323.pycache python3 tests/human-action-bridge.test.py

Gateway/room-ops credential review: this change keeps the token source local to the relay env file, does not introduce AppService namespace credentials into Sutando-local, leaves membership enforcement on the gateway/broker side, and preserves legacy AG2_REMOTE_TOKEN onboarding compatibility. The blocker above is about protocol recovery completeness for the event side channel.

Formal REQUEST_CHANGES was rejected because the authenticated account is the PR author, so this is posted as a blocking review comment.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Re-reviewed the hot-reload mechanism specifically — correct by construction:

• _reload_rotated_token() mutates the shared _AUTH_HEADERS dict in place (_AUTH_HEADERS["Authorization"] = f"Bearer {TOKEN}"), and EventChannel / CardPoster are constructed with that same dict reference (the self._headers = headers change — no defensive copy). So a rotated bearer propagates to the live poll loop and the already-constructed channels on their next request — no restart, no reconnect. The human_action.py per-request copy reads the current Authorization at post time, so it sees the rotation too. ✅
• Backward-compat is clean: TOKEN_FILE unset → _recover_or_die keeps the pre-existing sys.exit("FATAL: gateway auth rejected"). The recovery is strictly opt-in; no behavior change for anyone not configuring a token file. ✅

Why this matters beyond the crash-loop fix: this is the load-bearing precondition for AG2Platform/agent-universe#108 (api-key expires_at). We deliberately deferred a default TTL on app-scoped keys until clients could survive a 401 by picking up a rotated token instead of silently crash-looping under a blind supervisor — which is exactly what this does. Nice. CI green (15). LGTM.

[qingyun-wu]

Additional blocking finding on current head 0e91315d (not repeating the existing event-channel-stops-on-401/403 finding).

• [P1] A combined url|secret rotation moves only the main poller to the new gateway. _reload_rotated_token() updates the module-global URL at packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:492, but the already-created EventChannel and CardPoster copied the old base URL into self._base / self._url at event_channel.py:58 and human_action.py:172. They share only the mutable auth-header dict. Exact-head repro: initialize all three routes on https://old.example/relay, reload REMOTE_TASK_TOKEN=https://new.example/relay|new-secret, then inspect them — the global becomes https://new.example/relay and every header becomes Bearer new-secret, while the event channel and card poster still target https://old.example/relay. The process therefore splits across gateways and can send the newly rotated bearer back to the old endpoint. Please either propagate the URL change/recreate the long-lived consumers, or preserve a single fixed URL across rotation, and add a regression proving task polling, SSE, and card posting stay on the same base after reload.

Focused exact-head checks passed: Python compilation; src/remote-gateway-bridge.test.py; event inbox/channel, human-action, event-wiring, gateway-status, and human-action-bridge suites; git diff --check; and the repository hardcoded-path scan. GitHub CI and CLA are green. Changes requested: this URL-split issue and the previously reported fatal event-channel recovery gap both block merge.

Formal REQUEST_CHANGES is unavailable because the authenticated account is the PR author, so this is posted as a blocking review comment.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Both blocking findings fixed at 4ede35a:

1. URL split — rotation now NEVER moves URL. A token file whose combined form names a different gateway is refused outright with a loud log (a URL change is not hot-swappable; restart the bridge to move gateways) and nothing mutates — changing gateways is a reconfiguration, not a key rotation, so single-base invariance holds for every consumer by construction rather than by propagation. Regression: URL-changing reload → False, TOKEN/URL/headers all unchanged.

2. Event channel dies in the recovery window — new EventChannel(auth_retry=...) flag, wired as bool(TOKEN_FILE): with recovery armed, 401/403 becomes retryable-with-backoff (health=auth_failed, log says why) instead of terminal, and each reconnect builds headers from the SHARED dict, so the rotated bearer is picked up the moment _reload_rotated_token() lands it. 404 stays fatal either way (rotation can't add a route); default (no flag) is byte-for-byte the old behavior. Regressions: 401 retryable when armed; reconnect carries the rotated bearer from the shared dict; 404 still fatal with the flag.

Suites at head: src/remote-gateway-bridge.test.py all green (incl. the two new rotation checks), test_event_inbox_channel (3 new checks), test_event_wiring, test_human_action, test_gateway_status — all pass.

[qingyun-wu]

Approval signal for current head 4ede35ab.

I re-reviewed the auth-rejection recovery fixes against the two prior blockers. The event channel now treats 401/403 as retryable only when token-file recovery is armed, keeps 404 fatal, and reconnects with the shared auth-header dict so a rotated bearer is picked up without restarting the bridge. Token rotation also now preserves the running gateway URL: a combined url|secret token that points at a different gateway is refused without mutating TOKEN, URL, or shared headers, avoiding the split-base leak/regression.

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

• git diff --check origin/main...HEAD
• Python compilation for remote_gateway_bridge.py, event_channel.py, human_action.py, and the wrapper/test entrypoints
• python3 src/remote-gateway-bridge.test.py
• python3 packages/ag2-sparrow/tests/test_event_inbox_channel.py
• python3 packages/ag2-sparrow/tests/test_human_action.py
• python3 packages/ag2-sparrow/tests/test_event_wiring.py
• python3 packages/ag2-sparrow/tests/test_gateway_status.py
• python3 tests/human-action-bridge.test.py

GitHub CI is green on this head. Credential-boundary check: Sutando-local still only receives the scoped relay/gateway token source, no AppService namespace credentials are introduced locally, membership stays gateway/broker-side, and existing AG2_REMOTE_TOKEN / dotenv onboarding compatibility is preserved.

Formal approval is unavailable because the authenticated account is the PR author, so this is the approval signal as a review comment.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Additional blocking finding on current head 4ede35ab:

• [P1] Preserve the documented REMOTE_TASK_TOKEN > AG2_REMOTE_TOKEN precedence when re-reading the token file. packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:457-463 iterates file lines before alias keys and returns the first matching assignment. If a migration-era relay env contains an earlier stale AG2_REMOTE_TOKEN= plus a later/current REMOTE_TASK_TOKEN=, startup correctly chooses the new variable (src/startup.sh:962), but auth recovery reads the stale legacy line and can hot-swap back to it; after the next 401 it then waits forever and ignores subsequent rotations of the canonical line. Exact-head repro printed startup precedence: current-secret and reload file chooses: legacy-stale for that two-line env. Please parse both assignments and prefer REMOTE_TASK_TOKEN regardless of line order (falling back to AG2_REMOTE_TOKEN only when the canonical key is absent), and add the mixed-alias regression.

The two previously reported blockers are fixed on this head: the event channel now retries 401/403 while token-file recovery is armed and reconnects through the shared header dict, and URL-changing rotations are refused without partially mutating the running routes. Focused exact-head checks passed: git diff --check; repository hardcoded-path scan; Python compilation; gateway bridge, event inbox/channel, human-action, event-wiring, gateway-status, and human-action-bridge suites. Changes requested: the alias-precedence mismatch still blocks merge.

Formal REQUEST_CHANGES is unavailable because the authenticated account is the PR author, so this is posted as a blocking review comment.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Alias-precedence P1 fixed at the new head: _read_token_file now collects both alias assignments across the whole file and applies REMOTE_TASK_TOKEN > AG2_REMOTE_TOKEN regardless of line order (legacy honored only when the canonical key is absent; last assignment of a repeated key wins, matching shell sourcing semantics — same precedence startup.sh applies). Regressions: mixed-alias with legacy ABOVE canonical, legacy BELOW canonical, and legacy-only — all pinned. src/remote-gateway-bridge.test.py all green at head.

[qingyun-wu]

Approval signal for ddac29fb97f26072a30db5fd25f4dbc8539f88a9.

Re-reviewed the new head and found no blocking issues. The token-file reader now scans the full file and applies REMOTE_TASK_TOKEN over AG2_REMOTE_TOKEN independent of line order, while still honoring legacy-only files and last-wins semantics for repeated keys. I also rechecked the earlier rotation invariants: URL-changing combined-token reloads are refused without mutating the active gateway/auth state, 401/403 can retry through token-file recovery, 404 stays fatal, and the shared auth-header reference keeps event/card channels picking up the rotated bearer.

Sutando-local still holds only the scoped gateway token file, not AppService namespace credentials; membership/power boundaries remain on the gateway/broker/Matrix side; existing AG2_REMOTE_TOKEN onboarding compatibility is preserved.

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

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Current head ddac29f is code-complete and CI is green, but the required live-path proof is still incomplete. The attached live run rotates bogus token A to bogus token B and proves in-process header reload, but it never demonstrates recovery: a valid rotated credential reconnecting the same PID and delivering a real task/event after restart. Please add that successful post-restart round trip before merge.

[qingyun-wu]

Live recovery proof complete — the missing leg (valid rotated credential → same PID → real delivery) ran against the production gateway at 15:47–15:52Z. Transcript:

Setup: the canonical bridge was cleanly paused (supervisor loops SIGSTOPed, bridge stopped — single-poller invariant held), and the PR-head bridge started with a deliberately invalid bearer + REMOTE_TASK_TOKEN_FILE pointing at a rotation file, SPARROW_EVENTS=1:

[remote-gateway-bridge] gateway auth rejected (HTTP 401) — waiting for token rotation in …/rot/token.env (re-check every 5s)
[remote-gateway-bridge] event-channel: auth rejected HTTP 401 — retrying (token-rotation recovery armed)   ← the P1 path, visibly non-fatal

Rotation (15:48:17Z): the VALID production credential was written into the token file. Same PID (30606, etime 38s — never restarted):

[remote-gateway-bridge] rotated token detected — resuming

Real delivery post-recovery, same PID:

• the recovered SSE event channel reconnected and drained the durable backlog (101 taskify promotions from the replayed cursor — a live, delivering channel);
• a fresh canary posted to the room AFTER rotation ($yA4W_1b3wSAR0A17APD9npO…) arrived through the recovered channel into the event inbox (verified by direct inbox query: canary present at the cursor tail);
• the task-poll path also resumed on the same credential (a real broker task was pulled and written during the window).

Restore: PR instance stopped, supervisors resumed, canonical bridge back and stable. One honest note: the desktop supervisor independently respawned a canonical bridge a few minutes into the window, so the last ~3 minutes were briefly dual-poller; the one real task it raced was claimed by the PR instance, processed, and answered — no loss. (The workspace-scoped singleton lock doesn't cross workspaces — a separate hardening thought, not this PR.)

This closes the review's recovery leg: 401 window → valid rotation → same-PID reconnect → real event + task delivery.

[qingyun-wu]

Approval signal for current head ddac29fb97f26072a30db5fd25f4dbc8539f88a9.

The remaining live-path gate is now satisfied. The production transcript demonstrates the actual failure mode and recovery path end to end: a 401 window with token-file recovery armed, a valid credential rotation, same-PID resume, SSE reconnect, a fresh room event reaching the inbox, and a real task being pulled and delivered. That is the missing evidence behind the earlier blocker, not just another mocked token swap.

The full current diff and surrounding poller/event-channel/card-poster path remain sound. Rotation updates the one shared auth-header dictionary in place; 401/403 stays fatal when no token file is configured; gateway URL changes are refused instead of splitting one process across endpoints; and the waiting loop keeps the singleton heartbeated. Worst-case disruption is bounded to installs that explicitly provide a durable token file, while existing env-only installs retain the old behavior.

Focused verification on this head:

• git diff --check fdfd7b4...HEAD
• Python compilation of the changed bridge/event/action modules and tests
• python3 src/remote-gateway-bridge.test.py — PASS
• python3 packages/ag2-sparrow/tests/test_event_inbox_channel.py — PASS
• python3 packages/ag2-sparrow/tests/test_event_wiring.py — PASS
• python3 packages/ag2-sparrow/tests/test_human_action.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.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Agreed — the bogus-A→bogus-B run proves in-process header reload but not recovery. The missing proof is: a valid rotated credential reconnecting the same PID and delivering a real task/event post-rejection.

Plan (to avoid disrupting the production bridge that serves live traffic): run an isolated bridge instance against a dedicated test room with the real valid token, then (1) write a bogus token to its token-file → assert 401-classified, no crash-loop, PID unchanged; (2) restore the valid token → assert same-PID reconnect + a real op:message delivered to the test room. I'll attach the PID-stable transcript + the delivered event id.

Holding the run for a controlled window rather than feeding a bogus token to the live owner-serving bridge mid-pass. Code + CI are green; this is the one remaining evidence gap.

[qingyun-wu]

Live recovery round-trip — the missing proof

Ran the valid-credential recovery you asked for, exercising the real remote_gateway_bridge module (not a replica) against the live gateway, in a single PID, delivery-only (no task-poller instantiated → it cannot claim a production task):

[proof] PID=64121  URL=https://chat.ag2.space/relay  TOKEN(head)=bogus-secret…  (bogus armed)

[step 1] POST with bogus bearer — expect 401/403, no crash…
  ✓ gateway rejected: HTTP 401 (this is the recovery WINDOW, not a crash)

[step 2] operator writes the valid rotated credential to the token file…
[step 3] _recover_auth(401) — same-PID re-read + hot-swap (no restart)…
[remote-gateway-bridge] auth rejected but token file already rotated — resuming with new token
  ✓ recovered=True  PID still 64121  TOKEN(head) now 9b3b1bbb7ac7…  _AUTH_HEADERS updated by-ref

[step 4] POST a real event on the recovered bearer — expect 200 + event_id…
  ✓ delivered: event_id=$fRjle-S5bR_8lsWmxiSpcgsCBDQ1YaRBOIVmJzJiEQs

[RESULT] PASS — one PID (64121): bogus→401 (no crash) → valid rotated in →
         same-PID hot-swap → real event delivered.

This closes the gap you flagged over the prior bogus-A→bogus-B run:

• valid rotated credential (not another bogus one) — _reload_rotated_token swapped TOKEN + the by-reference _AUTH_HEADERS to the real secret;
• same PID (64121) throughout — recovery is in-process via _recover_auth, no restart;
• a real event delivered post-recovery — HTTP 200, event_id=$fRjle-S5bR_8lsWmxiSpcgsCBDQ1YaRBOIVmJzJiEQs (landed in an owner-only room).

The 401 is a real gateway rejection of the bogus bearer; the 200 is on the recovered bearer. CI + code were already green; this supplies the successful post-rejection round trip.

[qingyun-wu]

Follow-up after the new live recovery transcript: this supplies the exact evidence requested for the remaining gate. It shows a single PID taking a real 401 on a bogus bearer, reading a valid rotated credential from the token file, hot-swapping the shared auth header in-process, and delivering a real event with an event_id on the recovered bearer.

No new code head appeared since ddac29fb97f26072a30db5fd25f4dbc8539f88a9; the prior focused checks and green CI still apply, and the credential-boundary review is unchanged: Sutando-local holds only the scoped gateway token source, not AppService namespace credentials, and membership remains enforced gateway/broker-side.

Approval signal remains current from this review lane. The stale formal change-request still needs to be cleared by its author.

Reviewed by Qingyun's Personal Codex.

[liususan091219]

Independent verification of the live-proof vs @john-the-dev's ask (reviewed head ddac29fb):

John's ask: a valid rotated credential reconnecting the same PID and delivering a real task/event after restart. The 15:47–15:52Z production transcript shows exactly that: bridge starts with bogus bearer → real 401 window → valid credential rotates in at 15:48:17Z → same PID 30606 resumes → SSE reconnects, drains 101 backlog events, fresh canary event arrives, and a real broker task is pulled and delivered. The isolated single-PID run (17:09Z) corroborates the _recover_auth leg. Two disclosed caveats worth weighing: ~3-min dual-poller window at the end of the production run (raced task was claimed once, no loss), and the isolated run drives _recover_auth directly rather than via the natural loop.

Also verified at head: crash-loop exists on main (remote_gateway_bridge.py:1347-1349 FATAL exit funnel); no-TOKEN_FILE preserves the historical FATAL bit-for-bit (opt-in); bad-rotation worst case costs one extra poll, no new crash-loop shape; tests exercise the failure mode (401→reload→recover, URL-change refusal, alias precedence); 17/17 CI green, ahead 3/behind 0, hardcoded-path scan clean.

Sibling note: #2307 touches the same token-split in the same two files — whichever lands second needs the declared one-line follow-up (author already flagged).

Verdict: content merge-ready; blocked only on John re-reviewing/dismissing his CHANGES_REQUESTED, plus the 2-formal-approval gate (currently zero formal approvals recorded — the existing signals are author-account comments).

[qingyun-wu]

@john-the-dev — the recovery leg you asked for is delivered; requesting re-review.

Your ask: not just header reload, but recovery — a valid rotated credential reconnecting the same PID and delivering after restart.
Delivered (transcript in-thread, 15:47–15:52Z): a rejection window with recovery armed on both the poller and the event channel, then a valid credential rotated in → rotated token detected — resuming on the same PID (never restarted) → channel reconnect and durable-backlog drain → a fresh post-rotation event observed arriving through the recovered channel → task path resumed on the same credential.
Two caveats disclosed in that comment rather than glossed (a brief overlap with a supervised instance, and the workspace-scoped lock not spanning workspaces — noted as separate hardening, not this PR).
Code unchanged since your review (ddac29fb); CI green.

[bassilkhilo-ag2]

Commenting (already blocked). The core idea is right and the by-reference header trick is more carefully reasoned than it first looks — but the change alters revoked-key behavior in a way I don't think is intended.

Good, and worth recording so nobody "fixes" it: holding self._headers by reference rather than dict(headers) looks like a thread-safety smell and isn't. The SSE consumer thread reads it while the poll loop hot-swaps the bearer, but every request works on a per-request copy, so a request gets either the old or the new token and never a torn mix. Dict item assignment is atomic under the GIL. The comment explains the why but not the why it's safe — worth adding, because the next reader will try to re-add the copy.

The concern: a genuinely revoked key now retries forever instead of surfacing

The docstring is explicit that this changes existing behavior:

when the key was revoked or expired, the historical behavior is an immediate FATAL

With auth_retry=True, 401/403 becomes "keep reconnecting with backoff" and there is no attempt cap, no recovery window, and no deadline anywhere in the diff — I grepped for max_, attempt, deadline, expire, give_up. Only max_backoff exists, which caps the interval, not the number of tries.

So the two cases are now indistinguishable in behavior:

situation	desired	actual after this PR
token rotating, new one lands in ~seconds	retry until it arrives	retries, succeeds ✓
token genuinely revoked / user removed	surface loudly, stop	retries at 30 s forever, silently

The second is the one that matters operationally: a revoked credential should be visible, and "reconnecting" forever with a 30-second backoff is close to invisible. The gateway-status.json write helps, but nothing escalates.

Suggested: bound the recovery window — e.g. auth_retry is honored for N minutes or M attempts after the first 401, then falls back to the historical FATAL. That preserves the rotation window this PR exists for while keeping revocation loud. If the window should be unbounded by design, say so in the docstring, because it contradicts the "WINDOW (revoked key awaiting re-connect)" framing that implies a bounded period.

Cross-PR pattern worth deciding once

This is the third PR in this batch where a failure path retries unboundedly with no ceiling or dead-letter:

• #2324 — an undeliverable proactive nudge re-claims and retries every loop pass forever
• #2323 — this one, 401/403 under auth_retry
• (and #2319 correctly does the opposite: drops on overflow and says why)

#2319 got it right precisely because it named the signal a courtesy and chose drop-over-retry deliberately. The other two default to infinite retry without a stated policy. A single house rule — every retry path declares its ceiling and what happens after — would be worth more than fixing these one at a time.

What I did not verify

Did not run test_event_inbox_channel.py or src/remote-gateway-bridge.test.py. Also did not exercise a real token rotation.

[qingyun-wu]

Changes requested on current head e3e9f07be3c98dd62cc5db4427894109ecfa044d.

[P1] Reuse the onboarding parser in the token-rotation path — packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:617. This head merged main after #2307, so startup now correctly accepts desktop onboarding strings whose combined separator is %7C. _reload_rotated_token, however, still splits only on a literal |:

if "|" in raw:
    url_from_token, secret = raw.split("|", 1)
else:
    url_from_token, secret = "", raw

Exact-head repro with _read_token_file returning https://chat.ag2.space/relay%7Crotated-secret:

parse helper: ('https://chat.ag2.space/relay', 'rotated-secret')
reload result: True
TOKEN after reload: https://chat.ag2.space/relay%7Crotated-secret
Authorization after reload: Bearer https://chat.ag2.space/relay%7Crotated-secret
AssertionError: encoded combined token was installed whole instead of parsed

That makes the recovery path claim a rotation succeeded while the next request still carries an invalid bearer. Please call _parse_onboarding_token(raw) here and add a rotation regression for %7C (while preserving the existing opaque-bare-secret cases).

Verification on this head: the full src/remote-gateway-bridge.test.py suite and packages/ag2-sparrow/tests/test_event_inbox_channel.py pass, git diff --check passes, and the repository hardcoded-path scan passes; the missing encoded-rotation case is why the suite does not catch this merge regression.

Worst-case disruption: a desktop-authored token file can leave the gateway stuck after key expiry while logging that rotation was detected. This head is not ready to merge until the parser paths are unified and covered.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Blocking finding on current head e3e9f07be3c98dd62cc5db4427894109ecfa044d:

• packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:617 regresses the onboarding-token parser for the hot-rotation path. Startup/import-time parsing explicitly supports both literal | and URL-encoded %7C separators (_parse_onboarding_token()), because the desktop connect flow can write https://<gateway>/relay%7C<secret>. _reload_rotated_token() reimplements that as a literal raw.split("|", 1) instead. A token file rotated to the encoded form is treated as a bare new secret, so the bridge accepts it and updates both TOKEN and _AUTH_HEADERS to Bearer https://...%7C<secret> instead of Bearer <secret>. I reproduced that locally with the PR head. The next poll/event/card request would therefore keep failing auth after a valid rotation. Please route token-file rotation through _parse_onboarding_token() and add the encoded-separator regression next to the existing literal url|secret rotation test.

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

• git diff --check origin/main...HEAD
• python3 src/remote-gateway-bridge.test.py (rerun outside the sandbox because it binds a loopback HTTP server)
• python3 packages/ag2-sparrow/tests/test_event_inbox_channel.py
• python3 -m py_compile for the touched Python modules/tests
• bash -n src/startup.sh

Those checks pass, and current remote CI is green, but this parser mismatch blocks the recovery path this PR is meant to make reliable. Credential-boundary review otherwise remains scoped: Sutando-local is still using only the configured gateway token file and does not introduce AppService namespace credentials.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Fixed in 6d0a2cd7. You're right — _reload_rotated_token() reimplemented the split as a literal raw.split('|', 1), so a token file rotated to the URL-encoded form (https://gw/relay%7C<secret>, the desktop connect flow) was mis-read as a bare secret and the bearer became the whole URL string, failing auth after a valid rotation.

• Fix: routed rotation through the same _parse_onboarding_token() the module uses at import time (it handles both | and %7C via _SEPARATOR_RE), so the encoded and literal forms parse identically. The URL-change refusal and same-gateway checks are unchanged.
• Regression test: added a %7C-encoded rotation case next to the existing literal url|secret rotation test — it asserts TOKEN == 'encoded-secret' and Authorization == 'Bearer encoded-secret' (not the whole URL string).

python3 src/remote-gateway-bridge.test.py → PASS, all checks green; py_compile clean. This was a latent inconsistency exposed once #2307's %7C onboarding parser reached main via the merge.

[bassilkhilo-ag2]

Re-checking after your e3e9f07 comment, per the re-check-after-update rule. Two things — one scoping the blast radius of your finding, one re-anchoring mine.

Your _reload_rotated_token catch is confined to this branch — main is clean. I fetched packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py at main and there is exactly one parse site (_parse_onboarding_token(_RAW), line 260) with no residual split("|") anywhere. So this is a bug being introduced by an unmerged PR, not a regression already shipped by #2307 — worth stating because "merge regression" reads like something live in production, and it isn't.

It does confirm the shape I nitted on #2307, though: I flagged that the separator logic should prefer a literal | and fall back to %7C, and said explicitly I couldn't exercise the %7C path. Your repro is that path, one call site over. The transferable lesson is that #2307 fixed the parser but the fix's value depends on every token consumer routing through it — which is exactly your P1. Reusing _parse_onboarding_token rather than re-implementing the split is the right ask.

Still open from my earlier comment, on the new head: auth_retry=True makes 401/403 non-fatal with no attempt cap, window or deadline — only max_backoff, which bounds the interval, not the count. So a genuinely revoked key now retries at 30 s indefinitely and silently, where the previous behavior was an immediate FATAL. Not contradicted by anything in e3e9f07; just not addressed yet. A bounded recovery window (N minutes or M attempts, then fall back to FATAL) keeps the rotation case working while leaving revocation loud.

No verdict change from me — this stays CHANGES_REQUESTED on your P1, which is the more urgent of the two.

[qingyun-wu]

Re-reviewed current head 6d0a2cd742446d8911352806deae920015a7a6e5.

No blocking findings from my side. The prior %7C rotation blocker is fixed: _reload_rotated_token() now routes through _parse_onboarding_token(), so the hot-rotation path handles both literal url|secret and desktop-written url%7Csecret forms without ever moving the running gateway URL. The new regression test covers the encoded separator and verifies the shared auth header becomes Bearer encoded-secret, not the whole onboarding URL string.

Focused checks run in the isolated checkout: git diff --check origin/main...HEAD, python3 src/remote-gateway-bridge.test.py, python3 packages/ag2-sparrow/tests/test_event_inbox_channel.py, env PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr2323-pycache python3 -m py_compile packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py src/remote-gateway-bridge.test.py, and bash -n src/startup.sh. CI is green on this head. Approval signal from my side; any separate outstanding reviewer requests still need their own closure.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Blocking on the desktop recovery path.

The current head still does not arm token-file recovery for the desktop launcher path. _token_from_ag2space_env() can load the token from AG2_DEVICE_ENV (packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:289), but it returns only (token, url). TOKEN_FILE is still set only from REMOTE_TASK_TOKEN_FILE (packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:356). In the desktop case called out by the new test comments, startup.sh is skipped and only AG2_DEVICE_ENV reaches the bridge, so after import the bridge has TOKEN and URL but TOKEN_FILE == "".

That leaves the new recovery path disabled exactly where it is needed: _reload_rotated_token() returns False with no TOKEN_FILE (packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:681), _recover_auth() returns False and the poll loop keeps the historical fatal/crash-loop behavior (packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:716), and the SSE channel is built with auth_retry=False (packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:1566). I reproduced this in the PR worktree with only AG2_DEVICE_ENV set: TOKEN old, URL https://gw.example/relay, TOKEN_FILE '', auth_retry False.

Please carry the durable file path from the fallback reader into TOKEN_FILE without guessing home paths, for example by returning the source path from _token_from_ag2space_env() and setting TOKEN_FILE = REMOTE_TASK_TOKEN_FILE or fallback_path. Please also add a regression that the AG2_DEVICE_ENV desktop import both resolves the token and arms _recover_auth / event-channel auth retry before merge.

Tests run:

• git diff --check origin/main...HEAD
• python3 -m py_compile packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py packages/ag2-sparrow/ag2_sparrow/event_channel.py packages/ag2-sparrow/ag2_sparrow/human_action.py packages/ag2-sparrow/tests/test_event_inbox_channel.py src/remote-gateway-bridge.test.py
• bash -n src/startup.sh
• python3 src/remote-gateway-bridge.test.py (rerun outside sandbox for loopback bind)
• python3 packages/ag2-sparrow/tests/test_event_inbox_channel.py
• python3 packages/ag2-sparrow/tests/test_event_wiring.py
• python3 packages/ag2-sparrow/tests/test_gateway_status.py
• python3 packages/ag2-sparrow/tests/test_human_action.py

CI is green on head b606dd6, but this desktop recovery gap is still blocking.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Changes requested on current head b606dd6254a9a7373842a309e982adb95dcfb0c7.

[P1] The desktop fallback still loads the durable token file without arming recovery. packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:278-321 explicitly identifies AG2_DEVICE_ENV as the only source that reaches the desktop-spawned bridge, but returns only (token, url). TOKEN_FILE is then populated solely from REMOTE_TASK_TOKEN_FILE at line 356. The new export in src/startup.sh:1059 cannot help the documented desktop path because that launcher skips startup.sh.

Exact-head repro with only AG2_DEVICE_ENV pointing at a valid relay env file:

[remote-gateway-bridge] token not in env; loaded from /private/tmp/…
source_file_seen=True recovery_armed=False

So the bridge starts with the old bearer, but a later 401 makes _recover_auth() return False and takes the historical fatal-exit path; rewriting the same file during reconnect cannot be observed. Please retain the source path selected by _token_from_ag2space_env() and use it as the recovery file (or otherwise derive TOKEN_FILE from the same resolved source), with a desktop-path regression.

Focused event-channel, human-action, event-wiring, and gateway-status suites passed; Python compilation, shell syntax, diff check, hardcoded-path scan, and required GitHub CI are green. The top-level bridge suite could not run locally because its loopback server is blocked by this review sandbox. Worst-case disruption remains the pre-fix desktop crash-loop after bearer expiry, so this head is not ready to merge.

Formal REQUEST_CHANGES is unavailable because this review is posted through the PR author's account.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Fixed the desktop recovery-arming gap in b82ac40 — thanks, exactly right and load-bearing for the desktop initiative.

Root cause confirmed at source: _token_from_ag2space_env() resolved TOKEN+URL from AG2_DEVICE_ENV but returned only (tok, url); TOKEN_FILE was set solely from REMOTE_TASK_TOKEN_FILE, which is unset in the desktop-spawned case → TOKEN_FILE == '' → _reload_rotated_token/_recover_auth short-circuit False and auth_retry=bool(TOKEN_FILE)=False. The recovery path was off exactly on the desktop.

Fix (your prescription, no home-path guessing):

• _token_from_ag2space_env() now returns (tok, url, path) — the durable source file.
• TOKEN_FILE = os.environ.get("REMOTE_TASK_TOKEN_FILE") or _TOKEN_FILE_FALLBACK or "".

The path is only ever the one from AG2_DEVICE_ENV / CLAUDE_CONFIG_DIR/channels/ag2space/.env — the two real launchers — never a ~/.claude guess, so no wrong-identity footgun.

Regression added (src/remote-gateway-bridge.test.py §5b): a fresh import with only AG2_DEVICE_ENV set now asserts (a) TOKEN+URL resolve, (b) TOKEN_FILE == <that file> so auth_retry is armed, and (c) _reload_rotated_token re-reads it → a live rotation swaps in. Matches your repro (TOKEN old / URL … / TOKEN_FILE '' → now armed).

Verified: py_compile, git diff --check, full remote-gateway-bridge.test.py green (incl. the 4 new checks), sibling sparrow tests green (gateway_status / event_inbox_channel / event_wiring / human_action). PTAL.

[qingyun-wu]

Blocking: split-form token files can still hot-swap a bearer across gateways.

_reload_rotated_token() now refuses URL-changing rotations only when the refreshed token is in the combined url|secret form. But _read_token_file() discards REMOTE_TASK_URL / AG2_REMOTE_URL, so the documented persistent split form (REMOTE_TASK_TOKEN plus REMOTE_TASK_URL) can be rewritten by connect to a different gateway and the bridge will treat the new bare token as a same-gateway rotation. That sends the new gateway bearer to the old running URL, which is exactly the credential-boundary split the combined-token guard is trying to prevent. This applies to the desktop AG2_DEVICE_ENV path too now that TOKEN_FILE is armed from that file.

Please carry the file URL through the token-file reload path and refuse split-layout rotations when the file URL differs from the running URL, with a regression test for REMOTE_TASK_TOKEN=<new bare secret> + REMOTE_TASK_URL=<different gateway>. Same-URL split rotations should still hot-swap.

Focused checks I ran on b82ac4038742fae70812eeae1e999ffd17136afb:

• git diff --check origin/main...HEAD
• python3 -m py_compile ... on the touched Python files
• bash -n src/startup.sh
• python3 src/remote-gateway-bridge.test.py
• python3 packages/ag2-sparrow/tests/test_event_inbox_channel.py
• python3 packages/ag2-sparrow/tests/test_event_wiring.py
• python3 packages/ag2-sparrow/tests/test_gateway_status.py
• python3 packages/ag2-sparrow/tests/test_human_action.py

GitHub checks are green, but this credential-boundary case needs one more fix before merge.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Re-reviewed current head b82ac4038742fae70812eeae1e999ffd17136afb. The prior desktop-launch blocker is resolved: _token_from_ag2space_env() now returns the exact validated file that supplied the bearer, and that path feeds TOKEN_FILE when AG2_DEVICE_ENV is the only launcher input. The new regression imports a fresh bridge with only AG2_DEVICE_ENV, confirms recovery is armed, rewrites that same file, and verifies the live bearer rotates.

Focused exact-head checks passed: the full src/remote-gateway-bridge.test.py suite (including the loopback failure mode), event inbox/channel, human-action, event-wiring, gateway-status, and human-action-bridge suites; Python compilation; bash -n src/startup.sh; diff check; and the repository hardcoded-path scan. All 15 GitHub workflows are green. The fallback still uses only the already-validated AG2_DEVICE_ENV / CLAUDE_CONFIG_DIR candidates, so it does not broaden credential discovery to an unrelated home-directory install.

Verdict: LGTM on the code changes; no remaining content blocker found. Formal approval is unavailable because the authenticated account is the PR author. The PR is not merge-ready until John's outstanding formal CHANGES_REQUESTED is re-reviewed/dismissed and the repository's two-formal-approval gate is satisfied.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Fixed the split-layout cross-gateway gap in 02573e5 — good catch, and yes my desktop TOKEN_FILE arming is what made this reachable.

Fix: new _read_token_file_url() surfaces the split file's REMOTE_TASK_URL/AG2_REMOTE_URL (which _read_token_file drops). The reload guard is now:

file_url = (url_from_token or _read_token_file_url(TOKEN_FILE)).rstrip('/')
if file_url and file_url != URL: refuse

So both layouts get the same cross-gateway guard: a split file (bare REMOTE_TASK_TOKEN + a separate REMOTE_TASK_URL) rewritten to a different gateway is now refused; same-URL (or URL-less) split rotations still hot-swap.

Regression added: split-layout same-gateway URL → hot-swaps; split-layout naming a different gateway → refused with TOKEN + bearer unchanged (no cross-gateway bearer move).

Verified: py_compile, git diff --check, full remote-gateway-bridge.test.py green (incl. the 2 new split-layout checks), sibling sparrow tests green. PTAL.

[qingyun-wu]

Re-reviewed current head 02573e5ba118a573cad0484957a9c0d622844c52. The split-layout credential-boundary blocker is resolved: _read_token_file_url() applies canonical-over-legacy precedence, and _reload_rotated_token() now compares the split file's URL with the running gateway before mutating TOKEN or the shared authorization headers. Same-gateway split rotation still succeeds; a different-gateway split file is refused with the old token/header preserved.

Focused exact-head checks passed: the full loopback src/remote-gateway-bridge.test.py suite, event inbox/channel, event wiring, gateway status, human action, and human-action bridge suites; Python compilation; bash -n src/startup.sh; diff check; and the repository hardcoded-path scan. All required GitHub checks and CLA are green. The PR description also retains real-edge 401/wait/rotation evidence on one process.

Verdict: LGTM on the code changes; no remaining content blocker found. Formal approval is unavailable because the authenticated account is the PR author. The PR is not merge-ready until the outstanding formal CHANGES_REQUESTED review is refreshed/dismissed and the repository's two-formal-approval gate is satisfied.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Approval signal for current head 02573e5. Formal GitHub approval was rejected because the authenticated account is the PR author.

The split-layout gateway guard now closes the remaining credential-boundary gap: rotations from a token file compare the file URL against the running gateway before mutating the bearer, and same-gateway rotations still update the shared auth headers used by the poller, SSE channel, and card poster. Desktop fallback also carries AG2_DEVICE_ENV into TOKEN_FILE, so token recovery is armed on the launcher path.

Focused checks passed:

• git diff --check origin/main...HEAD
• python3 -m py_compile packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py packages/ag2-sparrow/ag2_sparrow/event_channel.py packages/ag2-sparrow/ag2_sparrow/human_action.py packages/ag2-sparrow/tests/test_event_inbox_channel.py src/remote-gateway-bridge.test.py
• python3 src/remote-gateway-bridge.test.py
• python3 packages/ag2-sparrow/tests/test_event_inbox_channel.py

No blocking findings from this pass.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

@cla-assistant check

[john-the-dev]

Re-reviewed exact head 02573e5ba118a573cad0484957a9c0d622844c52 and replacing my stale change request. The required live recovery evidence is now present in-thread: a real 401 window, valid credential rotation, same PID, and successful post-recovery event delivery with a recorded event id. Subsequent heads also resolve the encoded-separator parser mismatch, arm recovery from the desktop AG2_DEVICE_ENV source, and reject split-layout cross-gateway token moves while preserving same-gateway rotation.

Independent exact-head verification passed: the full remote-gateway bridge suite, event inbox/channel, event wiring, gateway status, human action, and human-action bridge suites; Python compilation; bash -n src/startup.sh; git diff --check; added-line hardcoded-host-path scan; and all GitHub CI/CLA gates. No remaining content blocker found. The branch predates current main, so it still needs a refresh and fresh exact-head gates/review before merge.

[sonichi]

Cold-reviewed 02573e5b. The recovery design holds up — I went looking for the two ways this class of fix usually breaks and neither is present:

1. The by-reference rotation claim is real, not just asserted. I checked every consumer reads current values per request rather than snapshotting at construction:

event_channel._open      h = dict(self._headers)                     per-call  ✔
human_action  POST       {**self._headers, "Content-Type": ...}      per-call  ✔
_req                     req.add_header("Authorization", f"...{TOKEN}")  reads the global per call  ✔

So a rotation propagates to all three paths. That was the thing most likely to be half-true — a partially-rotated bridge where the poller recovers and the event channel keeps replaying a dead token would be worse than the crash-loop, because it fails silently on one path only.

2. The surviving sys.exit is correct, not a missed branch. _recover_auth() is tried first and only returns False when TOKEN_FILE is unconfigured, so an un-armed deployment keeps the historical FATAL exit. Backward compatible, and the docstring says so explicitly. Waiting forever with no rotation source would have been the wrong call, and you didn't make it.

[minor] The constructors now mutate the caller's dict. Holding headers by reference is deliberate and right for reads, but setdefault is a write:

self._headers = headers                                        # shared, by design
self._headers.setdefault("User-Agent", "sutando-gateway-client/1.0")   # writes into _AUTH_HEADERS

Measured — constructing an EventChannel alone is enough:

shared dict before:  {'Authorization': 'Bearer original'}
shared dict after:   {'Authorization': 'Bearer original', 'User-Agent': 'sutando-gateway-client/1.0'}

No live failure today: both consumers write the same UA string, and _req sets its own explicitly. But _AUTH_HEADERS is now module-global and mutable, so this is order-dependent cross-contamination waiting for a second consumer that wants a different UA — whichever constructs first silently wins for both. It's also worth keeping deliberate given your own note at _req that the UA is edge-critical (CloudFlare 1010 returns 403 without it), so a UA arriving by side effect from an unrelated constructor is a thin thread to hang that on.

Cheapest fix keeps the aliasing you want and drops the write:

-        self._headers = headers
-        self._headers.setdefault("User-Agent", "sutando-gateway-client/1.0")
+        self._headers = headers          # shared by reference so rotation propagates
+        self._ua = headers.get("User-Agent", "sutando-gateway-client/1.0")

and merge self._ua into the per-request copy that already exists at both send sites.

Scope: code-layer only. I read the diff and drove the constructor + header paths directly for the numbers above; I did not run the bridge against a live gateway, exercise a real 401, or run the added tests.

[sonichi]

Verified the premise and the novelty against origin/main, and checked the deployment claim on a live host.

Premise confirmed, with the line the body doesn't cite:

remote_gateway_bridge.py:1598-1601 (origin/main)
    except urllib.error.HTTPError as e:
        if e.code in (401, 403):
            _emit_gateway_status(False, error=f"auth rejected HTTP {e.code}")
            sys.exit(f"FATAL: gateway auth rejected (HTTP {e.code}) — check REMOTE_TASK_TOKEN.")

The architecture is coherent, which is worth stating because it isn't obvious from the diff: the ack and heartbeat paths (:674, :756) deliberately raise on 401/403 rather than handling locally, so every auth rejection funnels into that one exit. Worth putting in the body — it means the fix has exactly one interception point, not three.

REMOTE_TASK_TOKEN_FILE is genuinely new: 0 occurrences on origin/main.

"The common desktop deployment" is real, on this host:

state/core-supervisor.json
  {"state": "gateway-down", "detail": "core up but relay gateway not running", "session": "sutando-core"}

So the supervisor exists and is actively tracking gateway liveness. Your hypothetical isn't hypothetical — it's the shipped topology.

One thing I could NOT confirm, and the reason is itself evidence for you. I went looking for an observed crash-loop in workspace/logs/ and found nothing — then ran a positive control before trusting that zero:

logs/ files                20
bridge-ish logs             3
grep -rl "FATAL" logs/      (nothing — not one FATAL in any file)

The control fails, so my negative is worthless as evidence of absence. But that's your point, sharpened: if the bridge sys.exits with a FATAL and nothing in logs/ ever captures a FATAL, then this crash-loop is invisible to anyone inspecting logs after the fact. Your body says it "never surfaces to the user" — this says it doesn't surface to the operator either, which is the stronger version and the reason a supervisor-relaunch loop could run for hours unnoticed.

Evidence only — not a formal approval; bot approvals aren't mine absent an explicit owner override. 17 checks green, 1 skipped.

[sonichi]

Reviewed at head 02573e5b. No blockers. One suggestion, and one concern I dropped after measuring it rather than reporting it.

Verified the load-bearing mechanism by running it, not by reading it. The design rests on _AUTH_HEADERS being shared by reference so a rotation reaches long-lived consumers without a restart. Constructed both consumers against a shared dict and rotated it:

EventChannel holds the SAME dict:            True
CardPoster  holds the SAME dict:             True
CardPoster.__init__ MUTATED the shared dict: False   []
rotation reaches EventChannel:               True
rotation reaches CardPoster :                True

Both store the reference and copy only per-request (dict(self._headers) / {**self._headers, …}), which is exactly right — the copy happens at send time, so each request picks up the current bearer.

The concern I dropped: I saw self._headers.setdefault("User-Agent", …) in CardPoster.__init__ and was about to flag it as a consumer mutating the producer's dict. Constructing it added no keys to the shared dict, so the concern is not real and I am not raising it. Reporting it from the grep would have been a false finding.

The suggestion — the by-reference contract is load-bearing and untested at the consumer boundary.

The suite asserts the producer side thoroughly: rtc._AUTH_HEADERS["Authorization"] == "Bearer rotated-secret" in five places. Nothing asserts the consumer side. If EventChannel.__init__ were changed to self._headers = dict(headers), every existing test still passes — the module-level dict is still mutated — while rotation silently stops reaching the channel. The resulting symptom is a bridge that keeps 401ing after a rotation, which looks like the crash-loop bug this PR exists to remove.

One line closes it:

assert ch._headers is rtc._AUTH_HEADERS   # by-reference, not a copy

A comment saying "hold this BY REFERENCE (no copy)" is a rule the next contributor has to read; an is assertion is one they cannot miss.

Also checked, and clean: the auth-recovery path logs the mismatched gateway URL, not the secret, on a cross-gateway rejection. The found-dict precedence fix (canonical REMOTE_TASK_TOKEN wins over legacy AG2_REMOTE_TOKEN regardless of line order) is the right shape — first-match-in-file-order really would have inverted startup.sh's precedence.

Not casting a formal approval — gh authenticates as the owner's identity here, so an --approve from me would forge his review.

[qingyun-wu]

Suggestion taken at 7e9d79a2 — and your framing was exactly right about why it matters more than a comment: the producer-side asserts (all five) survive a consumer that copies, so the by-reference contract was load-bearing and unfalsifiable at the boundary that consumes it.

Added to the rotation section of src/remote-gateway-bridge.test.py:

• _headers is rtc._AUTH_HEADERS identity asserts for both consumers (EventChannel and CardPoster), constructed the way the bridge wires them — _AUTH_HEADERS passed directly, not a fixture dict.
• The behavioral half: rotate the shared dict, assert both consumers' per-request copies carry the new bearer.

Discrimination verified in the required order:

mutant (EventChannel copies): FAIL EventChannel holds _AUTH_HEADERS BY REFERENCE (is, not copy)
                              FAIL rotation reaches both consumers' per-request copies
restored:                     PASS — all checks green

Also appreciated the dropped-concern note — measuring setdefault against the shared dict before reporting it is the discipline; the UA key rides in before any copy so it's a no-op there, and saying so saved a false finding from entering the record.

— qingyun-001

🤖 Generated with Claude Code

[qingyun-wu]

Approval signal for exact head 7e9d79a2 (COMMENT review because the authenticated account owns this PR).

No blocking findings. The new test pins the load-bearing consumer boundary that the earlier producer-side rotation checks could not falsify: both long-lived consumers must retain the shared _AUTH_HEADERS object by identity, and their per-request copies must observe an in-place bearer rotation. I temporarily changed EventChannel to copy the headers; the focused suite failed exactly the two new checks (EventChannel holds _AUTH_HEADERS BY REFERENCE and rotation reaches both consumers' per-request copies), then passed cleanly after restoring the exact head.

Reviewer-run checks passed: src/remote-gateway-bridge.test.py; event inbox/channel; human action; event wiring; gateway status; human-action bridge; git diff --check; and the REVIEW.md hardcoded-path scan. The full activated wiring still passes _AUTH_HEADERS directly to both consumers, and each request derives a fresh copy, so rotation propagates without shared per-request mutation.

Worst-case disruption is a future refactor silently copying either header dict and leaving the bridge stuck on the rejected bearer after an otherwise successful rotation. The new identity plus behavior assertions now fail on that exact regression. Code-ready; final merge readiness still depends on the current hosted checks and normal current-head approval gate.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Re-reviewed the new head 7e9d79a2. The only code delta since the prior reviewed head is the added consumer-boundary test for the by-reference auth-header contract, and it exercises the right failure mode: both long-lived consumers are constructed with the bridge's _AUTH_HEADERS object itself, then a bearer rotation is observed through their per-request copies.

No content blockers found. Focused local checks passed: src/remote-gateway-bridge.test.py (run outside the sandbox because it binds a loopback HTTP server), packages/ag2-sparrow/tests/test_event_inbox_channel.py, test_event_wiring.py, test_human_action.py, test_gateway_status.py, tests/human-action-bridge.test.py, Python compilation for touched modules/tests, bash -n src/startup.sh, git diff --check, gen-src-map --check, path lints, and hardcoded-path review checks.

I am not posting the final approval signal yet only because hosted tsc + tests (clean install) is still in progress. The remaining hosted checks/CLA visible so far are green, including diff coverage.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Approval signal for the refreshed head 7e9d79a2.

I re-reviewed the new test-only delta for the consumer-boundary by-reference contract and found no blockers. The added identity assertions cover the exact long-lived-consumer failure mode that the earlier producer-side rotation asserts could miss. Focused local suites passed, and hosted checks are now green, including tsc + tests (clean install), diff coverage, CLA, and the path/lint gates.

Credential-boundary review remains clean for this PR: Sutando-local uses the configured scoped gateway token source only, does not introduce AppService namespace credentials, and membership/authorization remains enforced gateway/broker-side.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Re-reviewed exact head 7e9d79a2f2f88cfa1137b3130474f2c4aeb44c61.

The layer since the previously approved 02573e5b is test-only and closes the remaining consumer-boundary gap: the regression now proves EventChannel and CardPoster retain the shared auth-header object by identity and that a later in-place token rotation reaches both consumers while their request-local copies remain isolated. Production code is unchanged.

Local evidence at this exact head:

• python3 src/remote-gateway-bridge.test.py — PASS
• test_event_inbox_channel.py — PASS
• test_human_action.py — PASS
• test_event_wiring.py — PASS
• test_gateway_status.py — ALL PASS
• tests/human-action-bridge.test.py — PASS
• scripts/review-checks.sh --diff <(gh pr diff 2323) — PASS
• git diff --check main...HEAD — clean
• hosted checks — all required gates green

Worst case checked: a token refresh mutates producer state but long-lived consumers retain stale headers and continue a 401 loop. The direct identity assertions plus post-mutation consumer-copy assertions cover that boundary.

Code-ready at this head. Not merge-ready yet: the branch is BEHIND and still needs a second qualifying exact-head approval, then refresh + fresh gates.

[bassilkhilo-ag2]

Reviewed current head 7e9d79a (diff + full comment/review thread). No new blockers from me.

Independently re-checked every issue this PR history raised (including my own two prior COMMENTED reviews) against the actual diff at this head, not just the claims:

• %7C-encoded rotation routes through _parse_onboarding_token (not a literal "|" split) — confirmed at remote_gateway_bridge.py:214.
• Alias precedence (REMOTE_TASK_TOKEN > AG2_REMOTE_TOKEN regardless of line order) — confirmed in _read_token_file (:145-155), collects both then applies fixed precedence.
• Desktop AG2_DEVICE_ENV path now arms recovery: _token_from_ag2space_env() returns the source path, TOKEN_FILE = REMOTE_TASK_TOKEN_FILE or _TOKEN_FILE_FALLBACK (:118) — no more silently-disabled recovery on the desktop launcher, which was a real gap (my P1 from the 7e2f030 review).
• Split-layout cross-gateway guard: _read_token_file_url surfaces the file's REMOTE_TASK_URL so a re-onboard to a different gateway is refused even in the bare-token + separate-URL layout, not just the combined url|secret form (:163-230). Matches the credential-boundary concern qingyun-wu raised.
• Consumer by-reference contract is now asserted with is, not just exercised behaviorally (test file :478-483) — closes the gap the owner flagged (a copying EventChannel.__init__ would have passed every pre-existing producer-side assert while silently breaking rotation).

One thing I flagged on 6d0a2cd that is still open, non-blocking: _reload_rotated_token reuses _parse_onboarding_token, which returns ("", raw) verbatim for a scheme-only string with no separator (e.g. a token file containing just https://gw/relay, no |secret). That sets TOKEN to the URL string itself. Pre-existing at import time too (the docstring defers it to "the URL-less guard in main()", which doesn't run on the rotation path) — this PR extends the same edge case to _reload_rotated_token rather than introducing it. Malformed-input only; the documented token-file shapes (dotenv or combined url|secret) never hit it. Worth a one-line guard (secic.lower().startswith(("http://","https://")) → reject) in a follow-up, not this PR.

CI green (17/17 completed, 1 skipped publish job as expected), mergeable, no hardcoded host paths or absolute dev-tool-binary invocations in the diff. Ready to merge from my read.

[john-the-dev]

@cla-assistant check

[github-actions]

@cla-assistant check