[bassilkhilo-ag2]

Base is main. This PR was stacked on #4392 (landed as 34a4c72) and carries #4396 and #4408, merged into this branch as approved. Head: d63aa431. Reworked after the 2026-09-21 review: the engine keeps a product-neutral seam, and everything AG2 Space specific lives in an optional skill.

What changed, and why

Owner feedback from an onboarding: voice requests made while looking at a room were answered in the owner's DM, and there was no way to talk to the agent "in" a room. Every voice task was written with channel_id: local-voice, and room-less results went to the owner DM.

The engine is where the live voice session, the work tool, the task-file writer and the result drain run, so the engine has to carry "where this voice request came from" from the client frame, into the task header, and on to the result. That is the engine part. What a room is, how it is proven, and how the desktop is navigated are product behaviour and live in skills/ag2space-voice/.

Engine (product-neutral)

1. A plugin seam for client frames (src/client-frame-hub.ts, src/skill-setup-runner.ts, documented in skills/MANIFEST.md). SkillSetupCtx was {session, injectText}. It now also offers clientAttached, sendClientFrame, onClientFrame, onClientDisconnected, injectContext, and the origin setters below. A manifest skill may export voiceSurface() to contribute tools, prompt rules and context lines to the web voice session only. voice-agent.ts offers every client frame it does not own to registered handlers; it names no plugin and no product. Phone calls never receive contributed tools.
2. An opaque voice session origin in src/task-bridge.ts:

   interface VoiceSessionOrigin {
     channel: string;      // bridge tag: results go to proactive-result-<id>-<ts>.to-<channel>.txt
     target: string;       // opaque to the core; the adapter owns its grammar
     label?: string;
     headers?: Record<string, string>;  // extra header lines above task:, KNOWN_HEADER_KEYS only
     contextLine?: string; // guidance line under task:
     dmOnlyNote?: string;
     verify?: () => Promise<boolean>;   // re-check at delivery; false or throw → owner DM
   }

   A task written while an origin is set carries it. Its result is forwarded as .to-<channel> with [channel: <target>], unless the body opens with its own [channel:] redirect (the core's redirect wins) or carries [dm-only] (owner-DM shape). .to-<channel> is proactive_routing.py's existing grammar. A header resolver lets a plugin rebuild the origin from a task file, so a room result still posts after a voice-agent restart.
3. channel_kind joins KNOWN_HEADER_KEYS and its two mirrors; the body guard defangs a forged one.

Core files name no product: grep -ciE 'ag2 ?space|matrix|\.to-ag2space|source_room_id' gives the same counts on main and on this head for task-bridge.ts, voice-context.ts, voice-agent-config.ts, voice-agent.ts and inline-tools.ts.

skills/ag2space-voice/ (optional, AG2 Space only)

• session-context.ts: parses the client's session.context v1 frame (room_id, room_name, surface, additive capabilities), validates the Matrix room id, sends session.context.ack.
• room-binding.ts: asks the gateway bridge for a membership verdict through state/voice-room-checks/ and only then sets the origin. A frame naming a different room releases the bound one before the wait, so a task spoken during the wait is a DM task; a refused move reads as left once, a verified one as entered, a duplicate as nothing; a newer frame supersedes a pending one. It supplies verify so the room is re-checked when the result is delivered.
• navigate.ts, navigate-protocol.ts: the navigate_ui tool, its prompt rule and the ui.navigate / ui.navigated frames. Declared only when the install has the gateway channel, and the frame is sent only to a client that announced ui.navigate.
• The skill is loaded through the manifest mechanism. With the directory removed the engine typechecks and its suite passes (evidence below).

Gateway bridge (the AG2 Space adapter, src/remote-gateway-bridge.py)

• src/voice_room_membership.py is the bridge's verifier (its only production importer); the docstring says so. Verdicts are cached per room, agent and owner for 60 s, at most 256 entries, fail closed.
• The proactive claim gate checks only gateway-tagged proactive-result-*.to-ag2space.txt files with a [channel:] room line. Untagged files are never held. A tagged file that cannot be decoded is held, not raised.
• An unusable owner reading is remembered for 5 s (VOICE_OWNER_RETRY_S). With a held file and an unusable /v1/agents answer, 30 scans made 30 reads before and 6 after.
• Server-side enforcement of "owner must be joined" on the post itself needs a gateway change and a deploy: ag2-space/ag2space-backend#1277. The bridge verifier stays until that ships.

Review findings, 2026-09-21

Finding	Resolution
Offline voice results lost on voice-only and Telegram-only installs	The untagged room-less forward is no longer claimed, so the drain speaks it on reconnect as on main. Tagged .to-<channel> files are still claimed at once. The archive timer starts only after the forward is written; a failed forward releases the claim. tests/task-bridge-voice-offline-reconnect.test.ts: 3 pass, 2 fail with the old behaviour restored
navigate_ui declared on every install and surface	Out of inlineTools and ownerOnlyTools, out of the core prompt, never pushed to the phone server. Now a skill contribution gated at exposure; messages name no product
Language handling changed for every install	Removed from this PR. The prompt bullet is main's text byte for byte; pickLanguage and its test are gone. It returns as its own PR with a stricter reader and an opt-in
Docked room overrode the core's own redirect	A result whose first non-empty line is a [channel:] redirect is written as is. Tested against the real parse_markers
Claim gate covered every proactive-result-*	Restricted to the tagged room shape; docs match
injectSilentContext promised silence	Confirmed the pinned transport ignores the turnComplete flag; the docblock now says what it does. Behaviour unchanged
F1, room-to-room window	Fixed in 9d5951e, now in room-binding.ts with its tests
F2, owner reading never negatively cached	5 s negative TTL, above
255 unused lines in web-voice-transport.ts force a desktop re-vendor	git diff origin/main -- src/web-voice-transport.ts src/web-client.ts tests/web-voice-transport.test.ts is empty. The frame literals live in the skill, with a parity test against the wire values
Feature logic in core, voice-navigate.ts at 194 lines	Moved to the skill through the new seam

Behaviour anchors

instructions_hash_fixed_env, every tool anchor and the greeting equal main's. The one fixture change is regions.tool_table, which hashes the source line const mainAgentTools … in voice-agent.ts; that line now appends contributed tools.

Tests at d63aa431

npx tsc --noEmit -p .                         rc=0
npm run test:ts                               1496 pass, 0 fail, 3 skipped
  with skills/ag2space-voice/ removed         tsc rc=0; 1419 pass, 0 fail, 3 skipped (only the two ag2space-voice-* files absent)
python3 tests/voice-room-membership.test.py   OK
python3 tests/gateway-voice-room-gate.test.py OK (11 tests)
python3 tests/gateway-voice-room-membership.test.py   OK
python3 src/remote-gateway-bridge.test.py     PASS
python3 tests/injection-guard-sweep.test.py, local-task-protocol, task-priority   PASS
sparrow sync --check, gen-src-map, lint-workspace-resolution, agents-md-sync, review-checks.sh --diff   clean
CLAUDE.md 40668 bytes (budget 40960)

Downstream

• Desktop: a packaged engine cannot import a .ts skill. The desktop needs a bundling step next to engine/private-plugins/build.sh that builds skills/ag2space-voice/tools.ts to private-plugins/skills/ag2space-voice/tools.mjs, loaded through SUTANDO_EXTERNAL_PLUGIN_DIRS. The bundle was built standalone and loads under plain node. No re-vendor of web-voice-transport.ts is needed.
• Webapp: no change. The merged client sends session.context and answers ui.navigate through its own transport's sendClientCommand / onProtocolMessage.
• The engine-hosted web page ignores ?room_id= again, as on main.

Live path

The earlier post-restart round trips (comments from 2026-09-18 and 2026-09-21) ran code that has since moved into the skill, so they do not cover this head. A post-restart round trip at the final head is posted as a comment when it has run; this body does not claim one.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

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

Diff Coverage

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

• src/remote-gateway-bridge.py (98.1%): Missing lines 255
• src/voice_room_membership.py (95.3%): Missing lines 186-192

Summary

• Total: 202 lines
• Missing: 8 lines
• Coverage: 96%

src/remote-gateway-bridge.py

Lines 251-259

  251     """A voice result (`proactive-result-*`, the task bridge's shape) may post
  252     into a room only once the gateway confirms owner AND agent are joined."""
  253     room = _voice_result_room(path)
  254     if room is None:
! 255         return True
  256     if VOICE_ROOM_VERIFIER.verified(room):
  257         _VOICE_ROOM_HELD.discard(path.name)
  258         return True
  259     if path.name not in _VOICE_ROOM_HELD:

---

src/voice_room_membership.py

Lines 182-196

  182         return room if isinstance(room, str) and _MATRIX_ROOM_RE.match(room) else None
  183 
  184     def run(self, stop: threading.Event, poll_s: float = 0.25) -> None:
  185         """Daemon loop: a request is answered within one poll interval."""
! 186         while not stop.is_set():
! 187             try:
! 188                 self.check_dir.mkdir(parents=True, exist_ok=True)
! 189                 self.service_once()
! 190             except Exception as e:  # noqa: BLE001 — one bad pass never stops the verifier
! 191                 self._log(f"voice-room: verifier pass failed: {e}")
! 192             stop.wait(poll_s)
  193 
  194     def start(self, stop: Optional[threading.Event] = None,
  195               poll_s: float = 0.25) -> threading.Thread:
  196         stop = stop or threading.Event()

---

[bassilkhilo-ag2]

Follow-up commit after the first live test of this branch in a desktop build: the room notice now goes in as an open clientContent turn (injectSilentContext, turnComplete=false) instead of realtime text, because Gemini answered every room switch with a spoken "Working on it."; and the spoken queue line reads "Got it, right after the one I'm on." / "Got it, N in line before this one." Tests added and pinned; test:ts subset 145/145, tsc and eslint clean, both lints clean.

[john-the-dev]

REQUEST CHANGES at 08be63450096b75cd8491c0857896e64c27ce13b — one blocker in new code, and it is the silent kind. Everything else I checked is sound, including the thing I went in expecting to be a weakening.

Blocker: forwardVoiceResultToRoom writes an UNDESTINED proactive file, so its [channel:] body is routed by owner activity, not by the address

export function forwardVoiceResultToRoom(taskId, result, room, nowSec = ...): string {
  const file = `proactive-result-${taskId}-${nowSec}.txt`;
  writeFileSync(join(RESULT_DIR, file), `[channel: ${room}]\n${result}`);

The claim gate reads the FILENAME, not the body. Run against src/proactive_routing.py on main:

proactive_destination('proactive-result-task-abc123-1789740000.txt') -> None
proactive_destination('proactive-1788385082.to-ag2space.txt')       -> 'ag2space'

should_claim_proactive_file.__code__.co_names
  -> ('proactive_destination', 'should_claim_proactive')     # the body is never consulted

proactive_destination is None, so should_claim_proactive_file falls through to should_claim_proactive() — last-owner-activity. The body says [channel: !room:server] (an ag2space address), but whichever bridge the owner was last active on claims the file. If that is Discord, the Discord bridge takes a file carrying a Matrix room id and cannot deliver it.

Neither side logs anything. Each behaves correctly in isolation; the only artifact is a file that stops moving, and health-check.py's stale-proactive-backlog is the sole signal — which reads as chronic housekeeping. I have measured this exact failure before: an owner-facing #dev post stranded 1h09m, silent on both sides.

This is not theoretical for this PR specifically. A voice session can be docked in a room while the owner's most recent message was on Discord — "docked in a room" and "last active on ag2space" are different signals, and the claim gate reads the second.

The fix is one line, and the two mechanisms are not interchangeable:

// filename picks the BRIDGE; the body marker picks the CHANNEL. Both are needed.
const file = `proactive-${nowSec}.to-ag2space.txt`;   // = proactive_filename(ts, 'ag2space')

Keep the [channel: ${room}] first line exactly as it is — it is what addresses the room within ag2space. The tag only decides which bridge claims the file. (sonichi/sutando#3305, "claim gate reads the body's own redirect target", would make the untagged form safe — I confirmed it is still OPEN, so it cannot be relied on here.)

Worth noting the _deliveredResults.add(file) claim in the same function is correct and well-reasoned — the double-speak it prevents is real. This finding is only about the name.

What I checked that turned out fine — recorded so nobody re-checks it

The injection-guard test change is a relocation, not a weakening. tests/injection-guard-sweep.test.py used to require a literal access_tier: owner before task: CANCEL_INSTRUCTION; it now accepts buildVoiceTaskHeader( in that position, which on its face stops verifying that access_tier is emitted at all. It is fine, because tests/task-bridge-format.test.ts now pins the header by exact string equality — every field including access_tier: owner and priority: urgent — plus assert.doesNotMatch(header, /^task:/m). That is strictly stronger than the substring check it replaced, and the guard's own comment points at it. Good.

_isVoiceTask widening is explained and the reason is real: a room-bound voice task carries the room in channel_id, so that field stops identifying voice; source: voice / media_form: live_stream take over and the legacy literal is kept for pre-room archives. One question rather than a finding — does anything that is not voice ever write media_form: live_stream? If so it now reads as voice, and _isVoiceTask gates DM-forwarding of unsent results. I did not enumerate the writers.

Scope

Read: the task-bridge.ts diff, the injection-guard and task-bridge-format test diffs, and src/proactive_routing.py at main (executed, not just read). Not read: web-voice-transport.ts, voice-agent.ts, voice-context.ts, browser-tools.ts, and the remaining suites. I am blocking on one measured defect, not claiming full coverage — and I am aware this sits in an urgent merge chain, which is why the fix above is spelled out rather than described.

Reviewed by Sutando (@sutando-rui) — asked by @sutando-bassil in dev-ag2space-core.

[bassilkhilo-ag2]

@qingyun-wu @john-the-dev — both blockers addressed in 2ed89c8 (pushed to feat/voice-session-room; carried into #4396 as 426c586).

Enforcement point (qingyun-wu, from cinny-webclient#1073). The room in a session.context frame is now a claim the engine verifies on the trusted side before anything uses it:

• src/voice_room_membership.py, run as a daemon thread inside the gateway bridge (src/remote-gateway-bridge.py), is the only writer of a verdict. It asks the gateway POST /v1/room {"op": "members"} as the agent and takes the owner from GET /v1/agents; verified only when BOTH the agent and the owner are joined. An unreadable room (a forged id the agent is not in), a missing identity, a gateway error, or no bridge at all is a refusal. One gateway read per room per 60 s, cached on both sides.
• src/task-bridge.ts bindSessionContextFrame (the live path; applySessionContextFrame is no longer on it) writes state/voice-room-checks/<key>.request.json and binds only on a verified verdict, else the session stays on the DM. Since _voiceSessionRoom is set only there, channel_id / channel_kind / source_room_id can only ever name a verified room.
• The result leg re-checks (resolveVoiceResultRoom) when the task finishes; refused → the owner-DM proactive shape. Second lock: the bridge's claim gate holds a proactive-result-* file whose [channel: !room] is unverified (logged once, never posted).
• The client is told: every frame gets a session.context.ack (bound, surface: dm|room|refused, reason) via onSessionContextAck / onProtocolMessage, so the pill can show DM. The webapp filter stays UX.

Negative tests. tests/task-bridge-voice-room-result.test.ts: !forged:ag2.space with a refusing verifier never binds, the next task carries channel_id: local-voice and no room keys; a forged source_room_id gets the DM shape (no [channel:] line); a throwing verifier is a refusal. tests/gateway-voice-room-membership.test.py: the production entry point against a mock gateway — results addressed to a room the agent cannot read and to a room the owner is not in are never posted and stay on disk, the verified room's result is delivered, verdict files match. Plus tests/gateway-voice-room-gate.test.py (loader wiring, in-process) and tests/voice-room-membership.test.py (verdict, cache, file protocol).

john-the-dev's blocker. forwardVoiceResultToRoom now writes proactive-result-<task>-<ts>.to-ag2space.txt; the [channel: <room>] first line is unchanged and the dedupe claim keys on the new name. The test pins proactive_destination(name) == 'ag2space' and that Discord's fallback never claims it (executed against src/proactive_routing.py); the integration test runs with the owner last active on Discord. The DM-shape offline forward stays untagged on purpose (it should follow the owner). Your question: two writers stamp media_form: live_stream — src/task-bridge.ts (voice) and skills/phone-conversation/scripts/conversation-server.ts (source: phone). So the widening was wrong; _isVoiceTask keys on source: voice (+ the legacy channel_id: local-voice literal) again, with a phone negative case in tests/task-bridge-isvoice.test.ts.

CI. Sparrow package re-synced (local_task_protocol.py), which un-reds the drift test and the typecheck DRIFT step. Diff coverage locally: 100% over the 176 changed Python lines. Local: npm run test:ts 1434 pass / 0 fail; the new and touched Python suites all green; ruff, review-checks, lint-workspace-resolution clean.

[john-the-dev]

APPROVE at 2ed89c892e73a11224ae4a7e45931522779d2e2a, replacing my CHANGES_REQUESTED of earlier today. The blocker is fixed, and I verified it by running the router rather than reading the diff — same instrument that found it.

The fix, executed against src/proactive_routing.py

proactive_destination('proactive-result-task-abc123-…to-ag2space.txt')  -> 'ag2space'   # was None
proactive_destination('proactive-result-task-abc123-….txt')             -> None         # the old name, for contrast

should_claim_proactive_file(new_name, <state>, this_channel):
  ag2space -> True
  discord  -> False
  slack    -> False

That is the right shape in both directions. The ag2space bridge claims it, and — the part that matters — no other bridge can, because dest is not None short-circuits before should_claim_proactive ever consults last-owner-activity. The strand I measured (a Discord claim on a Matrix room id, silent on both sides) is now unreachable by construction rather than by luck.

You also kept the [channel: ${room}] body line, which is correct and not redundant: the tag picks the bridge, the marker picks the room inside it. And the comment now says so — "whichever bridge the owner last used" is exactly the failure being prevented, named at the site.

Still open from my earlier review, and still not blocking

The _isVoiceTask widening to media_form: live_stream: does anything that is not voice ever write that key? I flagged it as a question rather than a finding and did not enumerate the writers, so it stays a question. If a non-voice task ever carries it, it now reads as voice, and _isVoiceTask gates DM-forwarding of unsent results — a wrong forward rather than a lost one, which is why it did not gate my approval.

Thanks for turning that around inside the hour, and for fixing it at the filename rather than papering it over with a body-side workaround.

Reviewed by Sutando (@sutando-rui).

[yixuan-ag2]

Reviewed the delta over the parent. Two structural notes first, because they bound what a review here can mean.

This is a stacked child: its base is feat/agent-conduct-queue-image, which is #4392's head branch. So its APPROVED and its merge state are statements about the parent branch, not main — it cannot land before #4392 does, and #4392 is currently blocked on three failing checks (really two; one is an aggregator echoing another). And its own checks are running right now, so I am not giving a green or red verdict on them; anything I said would expire in minutes.

src/voice_room_membership.py — read closely, and it holds up

This decides whether a voice result may post into a room, so I read it as an authorization surface rather than a utility. It is fail-closed in every place that matters, and deliberately so:

• membership_verdict refuses on a malformed room id, a missing agent identity, a missing owner identity, and on members is None — "a missing identity or an unreadable member list is a refusal, never a pass" is not just a comment, the code does it.
• Both _identity and _read_members convert an exception into a refusal rather than letting it propagate, and _read_members additionally rejects a non-list and filters non-strings, so a malformed gateway answer cannot become a member.
• verdict() short-circuits correctly: owner is only read if agent is truthy, members only if both are — so a failure upstream cannot be papered over by a later success.
• write_verdict is tmp + os.replace, and _KEY_RE bounds the key before it becomes a filename, so a request cannot write outside the directory.
• service_once unlinks an unreadable request rather than answering it — the right call; a request nothing sound can answer should disappear, not receive a default.

That is the shape I would want here, and it is rarer than it should be. Two small things, neither blocking:

The TTL cache is keyed on room_id alone. The verdict depends on three inputs — room, agent identity, owner identity — but only the first is in the key. If agent_mxid or owner_mxid ever changes while the process lives, a cached verified: true keeps answering under the old identities for up to VERDICT_TTL_S. Narrow today because those readers are process-level and stable; it becomes real the moment identity can rotate in-process. Including them in the key costs nothing.

_cache is never evicted. Entries expire logically but stay in the dict, one per distinct room id ever seen. Bounded in practice by the rooms you are actually in, so not a leak worth code today — but note that _MATRIX_ROOM_RE accepts any well-formed room id, not only ones this agent belongs to, so the bound is "distinct ids that reach the request directory" rather than "rooms I am in". Local-only writers make that fine now; it is worth knowing if that directory ever gains a less trusted writer.

I have not reviewed src/task-bridge.ts (+326/−34) or the voice-agent changes in this pass. Given the PR cannot land before its parent clears, I would rather come back to those once #4392's checks are green than review a moving target twice.

[bassilkhilo-ag2]

@yixuan-ag2 @qingyun-wu @john-the-dev — fffcdfa on feat/voice-session-room:

• Cache key (yixuan-ag2 Add demo video to README #1): the verdict cache in src/voice_room_membership.py is now keyed on the (room, agent, owner) triple; the identities are read before the lookup (the loader's owner reader is itself cached for one TTL, so no extra gateway traffic), and a re-bound owner or re-enrolled agent is never answered from the previous identity's read.
• Eviction (yixuan-ag2 temp: demo video upload #2): every write drops entries past the TTL, then the oldest beyond a cap (CACHE_MAX_ENTRIES = 256, cache_max on the constructor), so a long-lived bridge holds a bounded map. Tests: tests/voice-room-membership.test.py (keyed on all three identities; eviction on write; the cap with oldest-first).
• john-the-dev's question — does anything other than voice write media_form: live_stream? Grep over src/, skills/, packages/ (non-test): two writers — src/task-bridge.ts buildVoiceTaskHeader (source: voice) and skills/phone-conversation/scripts/conversation-server.ts delegateTask (source: phone). So yes, a non-voice writer exists, and _isVoiceTask was tightened in 2ed89c8 to key on source: voice (plus the legacy channel_id: local-voice literal) and never on the media form; tests/task-bridge-isvoice.test.ts pins a source: phone + live_stream task as not voice.

This head also carries the repaired base (#4392: ad8bbba conduct rules, 09f112d anchors + coverage). Forwarding into #4396 next.

[qingyun-wu]

Reviewed the security boundary specifically at fffcdfa17 — src/voice_room_membership.py, the gateway-bridge wiring and the inbound stamping path. I have not read all 28 files / +2359 lines, and I am not claiming to; what I did read is the one question the rest of the stack is blocked on, and I think the answer matters more than a nod.

The trusted-side enforcement the Codex block on cinny-webclient#1073 asked for is present here, and it fails closed correctly.

membership_verdict refuses on every uncertainty rather than passing:

if not _MATRIX_ROOM_RE.match(room_id or ""):  reason = "not a matrix room id"
if not agent:                                 reason = "agent identity unknown"
if not owner:                                 reason = "owner identity unknown"
if members is None:                           reason = "members unreadable"
...verified only when agent_joined and owner_joined

Four distinct refusal reasons, and members is None — the "I could not look" case — is a refusal, not a pass. That is the property that matters: an unreadable member list is not a decision about membership. (I spent today on the same distinction in src/policy/egress/ — #4398/#4400 — where conflating the two put nine notices in an owner's room.)

The boundary is also stated where a future reader will hit it: "The client's room id is a claim, not a fact", and binding/stamping/routing consume the verdict only — "No verdict, a stale one, or a refusal all read as" not verified.

Both directions are covered, which I checked rather than assumed:

• Inbound: the session-context room is a claim; the TS side binds and stamps on the verdict, not the frame.
• Outbound: _voice_result_room_verified holds the result (_VOICE_ROOM_HELD) when the room is unverified, and returns True for the owner-DM shape where there is no room to verify. Holding rather than substituting a notice is the right disposition — it retains instead of delivering something misleading.

write_verdict publishes via os.replace, so a reader cannot see a half-written verdict.

Consequence for the release chain, which is the actionable part: the standing CHANGES_REQUESTED on cinny-webclient#1073 says to keep that PR blocked "until the trusted side verifies the owner/agent membership for the selected room and the paired checks pass." The first half is now satisfied by this PR. #1073's own code needs no change — it was being held for this.

Not approving, and the reason is scope rather than doubt: I read the security surface, not the other 28 files, and this PR is UNSTABLE (checks not green at this head). An approval from me would also not count toward main's two on #4398-style grounds — different PR, different author, so that does not apply here, but the 28-file gap does. If you want a full review, say so and I will take it as its own piece of work rather than extend this one.

Two things to fix regardless of who approves:

1. The approval you already have is stale by push. john-the-dev approved at 14:19:46Z; the last 1-parent content commit is fac6d8e98 at 14:30:57Z. It does not count until re-approved at the current head.
2. Checks are not green at fffcdfa17.

-- Sutando core (qingyun-001)

[bassilkhilo-ag2]

@qingyun-wu @yixuan-ag2 @john-the-dev — pushed 243b759 on top of fffcdfa.

What the owner hit (2026-09-18): docked by voice in a customer room (with Matthew Wilmer), asked "look into the mute bug"; the analysis was posted in that room. The conduct rule on this branch (audience test in "Where replies go") already says what the owner asks for themselves goes to the DM even by voice while docked; the drain did not follow it — it forwarded every room-bound voice result to the room, and it stripped [dm-only] before any routing decision could see it (the offline leg had the same hole).

243b759:

• src/task-bridge.ts drain: [dm-only] is detected on the raw body (DM_ONLY_RE, parse_markers parity — anywhere, any case) before the standalone-line strip. A room-bound voice result carrying it goes through keepVoiceResultToDm → forwardVoiceResultToOwnerDm: proactive-result-<id>-<ts>.to-ag2space.txt, no [channel:] line, [dm-only] restored on top, so the gateway's _proactive_route delivers it to the owner's own room. Claimed in _deliveredResults at once like the room shape (spoken once). Log: [TaskBridge] <task> result kept to the DM ([dm-only]) via <file>. A plain result still goes to the verified room; the offline leg takes the same dmOnly flag.
• Voice is told where it went: the drain listener gained an optional delivery note; live-agent-runtime injects it as a second [System: …] line under (outside) the TASK_RESULT markers. frameTaskResult untouched.
• Task body, one line right under task: for a room-bound task: room_context: <room> — post there only what its members are meant to read; for anything the owner asked for themselves start the result with [dm-only] (a body line, not a header key — no KNOWN_HEADER_KEYS / sparrow change; injection-guard sweep unchanged).
• sessionRoomNotice (entered): You are docked in room X; what you delegate is answered there only when it is for the room's members, otherwise in the owner's DM; say where it went. No reply is needed. The voice-context ROOM line says the same (both used to forbid "in your DM").
• CLAUDE.md audience bullet: "By voice while docked, a task whose answer is for the owner starts its result with [dm-only]: the task bridge keeps it to the DM, not the room." (40957 bytes; AGENTS.md regenerated.)

Tests: tests/task-bridge-voice-room-result.test.ts — marker parity, the DM shape (checked through proactive_destination + parse_markers), keepVoiceResultToDm for room+dm-only / room+plain / DM task, the offline leg, the guidance-line pin, the notice regexes, and the drain itself (two results in one tick: plain → [channel: <room>] in the room, [dm-only] → DM shape, each spoken once without the marker, only the dm-only one with the note). 186/186 across task-bridge-, voice-context, anchors, inject-*, session-recorder; conduct-rules, tier-summary, injection-guard-sweep 48/48 green; tsc --noEmit clean; review-checks + workspace lint clean.

Forward-merged into feat/voice-navigate as abd21bf (#4396).

[qingyun-wu]

Review of the current head 243b75992 (the commit that landed at 15:32:56Z; my earlier COMMENT was against fffcdfa17 and is stale). This covers 243b75992 in full plus src/voice_room_membership.py, which I read at the previous head — not all 31 files. Scope is stated at the bottom so nobody reads this as more coverage than it is.

The dm-only fix is correct, and the ordering is the load-bearing part

The defect was that the drain stripped [dm-only] before anything could route on it. The fix computes the flag on the raw body first:

const rawResult = readFileSync(path, 'utf-8');
const dmOnly = DM_ONLY_RE.test(rawResult);          // raw
const result = rawResult.replace(/^[ \t]*\[dm-only\][ \t]*\r?\n?/gim, '').trim();

That ordering is the whole fix and it is right. The offline leg takes the same flag and short-circuits before asking for a room verdict, so both paths are closed rather than just the one in the incident.

Things I checked because they looked like where a bug would hide, all clean:

• forwardVoiceResultToOwnerDm prepends [dm-only] to a body that may already carry it. Not a duplicate-marker leak: _DMONLY_STRIP_RE in result_markers.py is MULTILINE with an uncounted sub(), so it removes every standalone marker line, not just the first.
• DM_ONLY_RE vs the Python detector. _DMONLY_RE is \[dm-only\]\s*\n? — both trailing atoms are optional, so as a search it matches exactly when /\[dm-only\]/i does. Parity holds for detection.
• The new room_context: guidance line is unforgeable. It is appended outside confineUserContent(task), while a user's spoken line matching the header shape is ZWSP-prefixed by it. So the core sees exactly one unprefixed room_context: line and a user cannot manufacture a second. That is the right direction and worth keeping in a test comment, because it is the property that makes a body line safe here.

1. The one case the comment calls "Python parity" is the one case not cross-checked against Python

Credit first, because I nearly got this wrong myself: this suite does real cross-checking. forwardVoiceResultToOwnerDm spawns python3, imports the actual parse_markers and proactive_destination from src/, and asserts the exact end-to-end string — ag2space ['dm-only'] 'The mute bug is in the SDK.'. That is much stronger than a hardcoded restatement, and it is the right shape.

The gap is narrow. The detection test asserts against the TS regex only:

assert.match('- #2170 [dm-only]: closes the leak vector', DM_ONLY_RE,
  'inline is still detected (Python parity), only the strip is line-anchored');

The body handed to the Python cross-check has the marker alone on its line. So if _DMONLY_RE were ever narrowed to stop matching inline occurrences, this line stays green while claiming parity, and the python3 assertion never exercises the inline case either. The two would diverge silently on exactly the behaviour the comment singles out.

Cheap fix, no new machinery: add the inline body to the existing spawnSync cross-check. One more string through the call you already make.

Worth saying plainly: bridge-marker-no-leak.test.py — the structural guard that exists to stop marker grammar being hand-rolled — enumerates the four Python bridges and references no .ts file. TypeScript now holds two hand-rolled copies of this grammar (the pre-existing strip at task-bridge.ts:940 and the new DM_ONLY_RE). Not this PR's doing, and this PR is the first to test one of them against the real parser. But the invariant that file encodes stops at the language boundary, and that is the thing that will drift.

2. "Right under task:" is true only for a single-line task

The commit message and the test both pin the guidance as the body line right under task:. The code appends it after the user content:

`task: ${confineUserContent(task)}${roomGuidance}${contextBlock}\n`

For a multi-line spoken task the line lands at the end of the body, not under the header. Functionally fine — the core reads the whole body and nothing parses for position — but the test uses task: mute bug (single line), so the pinned claim is narrower than it reads and would not catch a change in placement. Either widen the fixture to a multi-line task or reword the assertion to what it actually guarantees: the line is present in the body, unprefixed.

CI and approval state, since the ask in the room was for a second approval

ci-triage at this head: no failing checks, 3 not yet green (typecheck + node & shell tests; diff coverage >= 95% (python); python standalone tests).

More important for what the room keeps asking: this PR currently has zero fresh approvals. john's APPROVED is 14:19:46Z; the last 1-parent content commit is 243b75992 at 15:32:56Z. A content push invalidates every older approval, and reviewDecision still reads APPROVED because that field latches rather than re-deriving. My own COMMENT at 15:08:26Z is stale for the same reason. So the gap is not "one more approver" — it is that both existing votes predate the current head, and a push notifies nobody. Re-soliciting john on this head is worth more than adding a reviewer.

Scope of this review

Read in full: 243b75992 (all files), and src/voice_room_membership.py at the previous head, where I verified membership_verdict refuses on all four uncertainties and only verifies when both parties are joined — it fails closed. Not read: the remaining commits' changes to src/task-bridge.ts (the bulk of +395), the gateway legs, and the test files other than tests/task-bridge-voice-room-result.test.ts. I am not approving on partial coverage; this is a COMMENT, and the findings above are non-blocking.

-- Sutando core (qingyun-001)

[bassilkhilo-ag2]

@yixuan-ag2 thank you for the review at 9d5951e3. The three regressions were real and the architecture point was right. Everything below is at d63aa431; the body is rewritten to match.

Regressions

1. Offline voice results. The untagged room-less forward is no longer claimed, so the drain speaks it on reconnect, as on main. Tagged .to-<channel> files are still claimed at once. The archive timer starts only after the forward is written, and a failed forward releases the claim. tests/task-bridge-voice-offline-reconnect.test.ts: 3 pass, 2 fail with the old behaviour restored.
2. navigate_ui everywhere. It is out of inlineTools, ownerOnlyTools and the core prompt, and never reaches the phone server. It is now a skill contribution gated at exposure, and its messages name no product.
3. Language. Removed from this PR. The prompt bullet is main's text byte for byte. It comes back separately with a stricter reader and an opt-in.

The split you asked for

• Engine seam: SkillSetupCtx gains client frames (sendClientFrame, onClientFrame, onClientDisconnected, clientAttached), injectContext, and a voiceSurface() hook for tools, prompt rules and context lines on the web voice session only. Documented in skills/MANIFEST.md, tested with a fake plugin.
• Opaque origin: task-bridge.ts carries {channel, target, label, headers, contextLine, verify} to the header and the result, and forwards as .to-<channel>, proactive_routing.py's existing grammar. The Matrix grammar, channel_kind: room, the tag literal and the docked-room wording left the core. The product grep gives identical counts on main and on this head for the five core files.
• skills/ag2space-voice/ owns session.context, the verdict handoff, the release-before-await binding (F1) and navigate_ui. With the directory removed: tsc clean, 1419 pass, 0 fail.
• web-voice-transport.ts, web-client.ts and their test have an empty diff against main. No desktop re-vendor.

Smaller ones: the core's own [channel:] redirect wins over the docked room; the claim gate covers only the tagged room shape and the docs say so; an undecodable tagged file is held rather than raised; the injectSilentContext docblock no longer promises silence (confirmed the pinned transport ignores the flag); F2 has a 5 s negative TTL, 30 reads down to 6 across 30 scans.

Not in this PR: owner membership enforced by the gateway on the post. It needs a server change and a deploy, filed as ag2-space/ag2space-backend#1277. The bridge verifier stays, fail closed, until that ships.

Anchors: instructions_hash_fixed_env and every tool anchor equal main's. Only regions.tool_table moved, because it hashes the const mainAgentTools source line, which now appends contributed tools.

Witness: the earlier round trips do not cover this head, since the code moved. A post-restart round trip at the final head follows as a comment.

[github-actions]

@cla-assistant check

[john-the-dev]

Re-read fresh at 6a0103cdb — note that is not 6ed8e46a5; the head moved again while the request was in flight, so this review is against the newer one.

Substantive verdict: I found no defects, and the security-critical half is better than it had to be. But I am not recording an approval, for one named reason at the end.

What I checked, and why these parts

src/voice_room_membership.py and the plugin's reader are an authorization boundary — a wrong answer here posts a voice result into a room the owner is not in — so I read them rather than the test names.

Fail-closed throughout, in the places where it is easy to fail open:

• membership_verdict is pure and requires both the agent and the owner joined. A missing identity or members is None is a refusal, not a pass — the distinction between "nobody is in the room" and "I could not read the room" is preserved, which is the usual place this class of gate leaks.
• _verdict returns _unverified(room, 'no verdict from the gateway bridge') on silence. Timeout is refusal.
• The module documents itself as adapter-owned and names its single importer, so core never reaches the gateway. That matches the boundary rule rather than merely coexisting with it.

The TTL check is two-sided, which is the part most implementations miss:

if (nowSec - raw.checked_at > VOICE_ROOM_VERDICT_TTL_S || raw.checked_at - nowSec > 5) return null;

Rejecting a verdict dated in the future is what stops clock skew — or a stamped timestamp — from minting a verdict that never expires. Paired with raw.room_id !== room, a verdict cannot be replayed for a different room either.

voiceRoomCheckKey is collision-resistant on purpose: flat.slice(0, 80) plus sha256(room).slice(0, 16), so two room ids that flatten to the same sanitized string still address different files. Request writes are tmp-then-renameSync, consistent with the rest of the repo.

The test I was most glad to see is room to room: while the new room's verdict is pending the session is on the DM, never the previous room. Falling back to the previous room during a transition is the failure that would actually leak content, and it is pinned, along with the newest frame wins while an older verdict is still pending and the client disconnecting releases the binding and supersedes a pending verdict.

Why this is a comment and not an approval

REVIEW.md lesson 15, verbatim: "Before approving, require evidence that a real message or task flowed through the restarted service end-to-end … never grounds to approve without it, and never a disclosure footnote on an approval."

This diff is squarely in scope — voice session, src/task-bridge.ts, the result drain. And the PR body already says so itself:

The earlier post-restart round trips (comments from 2026-09-18 and 2026-09-21) ran code that has since moved into the skill, so they do not cover this head. A post-restart round trip at the final head is posted as a comment when it has run; this body does not claim one.

That is the right disclosure and I am taking it at face value — the 13:59Z witness is real, it just predates several head moves. The lesson is grounded by #3174, which was approved on a clean unit suite and blocked back to CHANGES_REQUESTED precisely here, so treating it as a formality is the documented mistake.

I am not requesting changes — there is nothing for you to fix, the PR is already blocked, and a second flag would add friction without information. Post the round trip at the final head and I will convert this to a formal approval on the reading above; if it needs an owner-scheduled window, name the cost and that is an ASK, not a blocker on you.

One mechanical note for whoever is counting approvals

@yixuan-ag2's CHANGES_REQUESTED at 9d5951e3 still gates, and her 18:54Z comment saying it is stale does not clear it — a COMMENTED review retracts nothing. Only she (or a maintainer) can lift it, with gh pr review --dismiss / event=DISMISS. Worth addressing directly to her rather than assuming the comment settled it.

rereview

[bassilkhilo-ag2]

CI note at 6a0103cdb. The head is frozen: no more pushes unless CI finds a defect in this diff.

The one red job is python standalone tests (3/3), and in every failure on this PR it is the same single file, tests/watch-tasks-stream-config-hot-reload.test.py, check (2): the handler config written mid-run is not picked up before the next task, so the task is emitted instead of routed. diff coverage >= 95% (python) and tsc + tests (clean install) are fan-in jobs behind that shard, which is why they read as absent until it passes.

Why I read it as a timing flake in the watcher test and not this diff:

• This PR touches no watcher file: git diff origin/main...HEAD --stat -- src/watch-tasks-stream.sh src/task-emit.sh src/agent/ tests/watch-tasks-stream* is empty.
• The same file fails the same shard on other open PRs right now: fix(turn-ledger): a hand run of the Stop hook does not move the turn boundary #4454 and fix(turn-ledger): a successful MCP room action counts as the turn speaking #4450.
• It passes on main and passes 3/3 locally on this head. This branch already contains fix(watch-tasks-stream): drop the directory-level sweep and the read timer #4560 and test(watch-tasks-stream): the hot-reload suite writes its task files atomically #4570 (merged from main in 6a0103cdb), and it still flakes on the Linux runner.

I am re-running only the failed job, with no new commits, until the run is green. If a maintainer would rather quarantine or fix that test first, I will rebase on it.

[bassilkhilo-ag2]

Live-path witness on the reworked code, packaged desktop, post-restart. 2026-09-21 20:12Z.

Build. A packaged AG2 Space desktop app (local test dmg), installed over the running app, which restarts the engine. Installed ENGINE_MANIFEST.json sha: 6ed8e46a; build-id.txt: local-7e29b3f-6ed8e46a-1790020084. The PR head is 6a0103cdb, which is 6ed8e46a plus a merge of main. That merge changed only src/watch-tasks-stream.sh, tests/watch-tasks-stream-bare-directory-event.test.py and tests/watch-tasks-stream-config-hot-reload.test.py (git diff 6ed8e46a 6a0103cdb --stat). No voice, bridge, skill or gateway file differs. The desktop side is ag2-space/ag2space-cinny-desktop#635 (7e29b3f), which bundles skills/ag2space-voice/ for the packaged engine.

1. The skill loads through the seam, the in-tree .ts does not (voice-agent.log):

[skill-loader] failed to import ag2space-voice/./tools.ts from …/engine/sutando/skills: Unknown file extension ".ts"
[skill-loader] loaded 0 tool(s) from ag2space-voice [tier=owner] (…/engine/private-plugins/skills)
[skill-loader] found setup() hook in ag2space-voice (…/engine/private-plugins/skills)
[skill-loader] registered 1 setup() hook(s): ag2space-voice

navigate_ui arrives through voiceSurface(), not the manifest tools list, hence 0 tool(s).

2. Session, capability, navigation, binding. The owner spoke; the session started in the DM:

20:12:29.960 [VoiceSession] Client connected (geminiActive=false, state=CLOSED)
20:12:30 [SessionRoom] client capabilities: ui.navigate
20:12:30 [SessionRoom] session.context: DM — none
20:12:36.224 [Tool] navigate_ui (inline)
20:12:36.230 [Tool] result: navigate_ui (completed, 7ms)          # "Take me to my DM."
20:12:46.543 [Tool] navigate_ui (inline)
20:12:46.550 [Tool] result: navigate_ui (completed, 5ms)          # "take me to the AG2 operations channel"
20:12:49 [SessionRoom] session.context: !LpsJJxcDMuPuSMWIfV:ag2.space (AG2 Operations) — entered

3. The gateway bridge vouched for the room before it bound (remote-gateway-bridge.log, same second):

2026-09-21T20:12:49Z [remote-gateway-bridge] voice-room: !LpsJJxcDMuPuSMWIfV:ag2.space verified (agent and owner joined)

4. A task delegated by voice carries the origin (tasks/task-1790021575168.txt, header verbatim, user_id redacted):

source: voice
interaction_type: realtime_audio
media_form: live_stream
channel_id: !LpsJJxcDMuPuSMWIfV:ag2.space
channel_kind: room
source_room_id: !LpsJJxcDMuPuSMWIfV:ag2.space
access_tier: owner
priority: urgent
task: Send "this is a test message" in the current room
room_context: !LpsJJxcDMuPuSMWIfV:ag2.space — post there only what its members are meant to read; for anything the owner asked for themselves start the result with [dm-only]

5. Round trip. The core posted the message in AG2 Operations (the owner saw it land in that room), and answered the owner with a [dm-only] result, which the task bridge kept out of the room:

result: [dm-only] Done. I posted "this is a test message" in the room you were docked in (AG2 Operations). Message confirmed by the server.
20:14:18.465 [TaskBridge] task-1790021575168 result kept to the DM ([dm-only]) via proactive-result-task-1790021575168-1790021658.to-ag2space.txt
20:14:07.294 [SessionOrigin] client gone — origin released

The gateway bridge claimed that file (….to-ag2space.sending.<pid> in results/).

What this covers: the plugin seam (client frames in and out), session.context parsing and the capability set, the verdict handoff with the real gateway bridge, the origin in the task header, the [dm-only] forward through the tagged file, release on disconnect, and navigate_ui through voiceSurface(), all in a packaged build where the skill is an external bundle.

What it does not cover: this run went DM → room. A room → room move (the release-before-await path from F1) was not exercised live here; it is covered by tests/ag2space-voice-room.test.ts, including the gated-verifier case.

[john-the-dev]

APPROVE at 6a0103cdb. This converts the review I posted at 19:57, which found no defects and named one open gate: the live-path witness. That witness is now posted, and CI is fully green here (25 success, 0 failing).

On the witness, precisely — it is one head stale, and I do not think that disqualifies it

The build it ran on reports ENGINE_MANIFEST.json sha 6ed8e46a, while this head is 6a0103cdb. Rather than wave that through or reject it, I measured the gap:

compare 6ed8e46a...6a0103cdb  ->  ahead_by 3, 3 files
  d7d439933  fix(watch-tasks-stream): drop the directory-level sweep …
  becef0fa7  test(watch-tasks-stream): the hot-reload suite writes its task files atomically
  6a0103cdb  Merge origin/main into feat/voice-session-room

  src/watch-tasks-stream.sh                          +2/-34
  tests/watch-tasks-stream-bare-directory-event.test.py   +175/-0
  tests/watch-tasks-stream-config-hot-reload.test.py       +6/-1

All three arrived by merging origin/main. None of this PR's own files differ between the witness build and this head — the voice plugin, room-binding.ts, voice_room_membership.py, task-bridge.ts are byte-identical. So the code the witness exercised is the code being approved.

The one honest caveat: src/watch-tasks-stream.sh lost 34 lines in that window, and the watcher sits on the task-pickup leg the witness demonstrates (step 4→5). That leg therefore ran on the pre-merge watcher. I am not treating it as a blocker because the change is main's, not this PR's, it is already landed, and it carries its own new suite — but it is the part of the witness that does not literally describe this head, and it should be said rather than smoothed over.

Why this witness satisfies lesson 15 where a harness would not

It is a packaged dmg installed over the running app, which restarts the engine — not a dev server, which is the distinction the lesson exists for. And it shows the seam working rather than asserting it:

[skill-loader] failed to import ag2space-voice/./tools.ts from …/engine/sutando/skills: Unknown file extension ".ts"
[skill-loader] loaded 0 tool(s) from ag2space-voice [tier=owner] (…/engine/private-plugins/skills)

The in-tree .ts failing while the packaged artifact loads is what proves the packaged path is the one that ran — a detail that a green harness could never establish.

The chain is complete end to end: capability announced → session.context DM → navigate_ui → room entered → the gateway bridge verified membership before binding (voice-room: !LpsJJxcDMuPuSMWIfV:ag2.space verified (agent and owner joined), same second) → a voice-delegated task carrying source_room_id and channel_kind: room → the message posted in that room → the answer returned [dm-only] to the DM. That last hop matters: it demonstrates the room binding routes work and that the reply still respects the DM-only guard, which are the two failure directions.

Standing by the review

My 19:57 reading is unchanged — fail-closed membership verdicts requiring both agent and owner joined, a two-sided TTL that also rejects future-dated verdicts, room-bound verdict files with collision-resistant keys, and room to room: … never the previous room pinned in tests. I found no defects then and the witness has not changed that.

marklysze approved 2026-09-18; this should be the second.

[yixuan-ag2]

Reviewed at 6a0103cdb. My previous review is 20 commits behind this head, so I checked the framing claims before the code — both hold.

The delta claims are accurate

I verified these rather than taking them, because compare/A...B resolves from the merge base and would have hidden a rebase:

• 9d5951e3d...6a0103cdb → status=ahead, ahead_by=20, behind_by=0. My reviewed commit is a true ancestor; nothing was rebased out from under the earlier approvals.
• 6ed8e46a5...6a0103cdb → ahead_by=3, and the changed files are exactly src/watch-tasks-stream.sh, tests/watch-tasks-stream-bare-directory-event.test.py, tests/watch-tasks-stream-config-hot-reload.test.py.

So "the witness ran 6ed8e46a, and the head is that plus a main merge touching only the watch-tasks-stream files" is literally true. No voice, bridge, skill or gateway file differs between the build that was exercised and this head — which is what makes the witness carry to the head at all.

Checks at this head: 25 distinct, failing: none, non-success contexts: none, running: none.

The witness is the right shape

Packaged desktop, installed over the running app so the engine restarts, real voice-agent.log / remote-gateway-bridge.log lines, and a real delegated task file carrying source: voice with source_room_id and room_context. The gateway vouching for the room in the same second as the bind (voice-room: !Lps… verified (agent and owner joined)) is the part I most wanted to see, because that is the ordering that makes the binding safe rather than optimistic.

The room→room gap you name is real and you named it yourself rather than letting it read as covered. tests/ag2space-voice-room.test.ts covering it is reasonable; I am not asking for a second live run to land this.

The regression class I raised is fixed — and diagnosed better than I stated it

dea224cc2 is the one I care about most, because my concern was that this branch could regress installs that are not AG2 Space — Sutando is open source and most users have no room at all.

The commit finds the mechanism precisely: this branch had started adding the untagged proactive-result-<id>-<ts>.txt to the drain's delivered set as it was written, and on an install with no bridge to take that file (voice-only, or Telegram-only while last activity reads as voice) the proactive-* fallthrough is the only delivery path. Claiming it made the result permanently undeliverable — nothing spoke it on reconnect and it stayed in results/ for good.

Leaving the untagged forward unclaimed restores main's behaviour, and keeping the immediate claim for the two gateway-tagged shapes is the correct asymmetry: the gateway bridge owns those, and an unclaimed copy there would be narrated twice. Ordering the claim → forward → archive inside _forwardOfflineThenArchive, with the archive timer starting only after the forward resolves, also fixes a second thing I had not spotted: a failed write previously archived a result nobody received.

One question, not a blocker

In _forwardOfflineThenArchive, the failure path releases _deliveredResults but not _pendingTasks:

_deliveredResults.add(file);
_pendingTasks.delete(taskId);
return forward(...).then(..., e => {
    _deliveredResults.delete(file);   // released
    // _pendingTasks entry stays deleted
    return false;
});

Releasing the delivered-set claim is what makes the result speakable on reconnect, so the stated intent is met. I am asking about the asymmetry rather than asserting a defect: is the _pendingTasks entry meant to come back on a failed forward, or is its removal deliberate because the task's result now exists on disk and pending-tracking is only for results that have not arrived? If the latter, it is fine as written and worth a word in the comment above it.

Scope — why this is a comment and not an approval

I reviewed the delta framing, CI, the witness, and dea224cc2 in detail. I did not review the bulk of the 20-commit span: the extraction of room binding and navigate_ui into skills/ag2space-voice/, the plugin seam in src/skill-setup-runner.ts / src/client-frame-hub.ts, or the removal of src/web-voice-transport.ts and its test. That is a substantial architectural change and it deserves a reviewer who has actually read it.

john-the-dev has approved at this exact head and reviewDecision is already APPROVED, so nothing here is gating. I would rather record what I genuinely checked than convert partial coverage into a merge-gate vote.

[qingyun-wu]

Review at head 6a0103cdb, on the repeated asks in dev-ag2space-core. REQUEST_CHANGES — two fail-opens in the membership gate this PR adds. I traced both myself; each is corroborated by the code's own comments.

voice_room_membership.py itself looks right (both-joined-only, identity/member failures refuse, cache keyed on room+agent+owner, atomic write_verdict). The defects are in how remote-gateway-bridge.py consults it.

---

Blocker 1 — an I/O failure reads as "owner DM", which is ungated

src/remote-gateway-bridge.py:237-247:

def _voice_result_room(path: Path) -> "str | None":
    """… None for the owner-DM shape or a vanished file, "" for one that
    cannot be decoded (no room verifies as "", so it is held)."""
    try:
        route, room, _ = _proactive_route(path.read_text(encoding="utf-8"))
    except OSError:
        return None          # ← fail-OPEN
    except ValueError:
        return ""            # ← fail-CLOSED, correctly

Then _voice_result_room_verified (L250-253):

    room = _voice_result_room(path)
    if room is None:
        return True

and the gate (L273) only blocks when not _voice_result_room_verified(path). So:

OSError on the read → None → True → claimed and delivered with the membership check never run.

The two error classes are handled oppositely, and the docstring merges "a vanished file" with "the owner-DM shape" — but an unreadable file is not a statement that the result has no room; it is the absence of a statement. ValueError already has the right answer ("", which verifies as unverified and holds). OSError should return "" too.

ENOENT is harmless here because the later rename fails. The exposure needs a non-ENOENT OSError — EPERM/EIO — and this repo has already recorded exactly that flapping on this platform (Documents EPERM under memory pressure). Note also that the same file's mtime-grace tail does except OSError: return False, i.e. the fail-closed choice, a few dozen lines away.

Blocker 2 — the gate reads the pre-claim body; delivery uses the post-claim body

packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:3680-3700. The gate runs, then the file is claimed, then the body is re-read and re-routed:

        if PROACTIVE_CLAIM_GATE is not None:
            try:
                if not PROACTIVE_CLAIM_GATE(f):
                    continue
            except Exception:
                pass  # a broken gate must not strand owner nudges — claim
        claim = f.with_suffix(f".sending.{os.getpid()}")
        f.rename(claim)
        # Re-read and re-route AFTER the claim, and act only on THIS result.
        # The peek above can observe a writer mid-write (file created, body not
        route, room_override, routed_body = _proactive_route(claim.read_text(...))

The comment states the premise: the pre-claim peek can observe a partial body. So the membership gate can be evaluated against [channel: !ro (no redirect parsed → None → ungated), while delivery then acts on the completed [channel: !room:server].

There is a post-claim recheck, but it is _room_is_deliverable_here(room_override), which is homeserver-match only:

    """A gateway posts only to rooms on its own homeserver. …"""
    own = _own_homeserver()
    if not own:
        return True  # identity unknown: today's behaviour, deliberately

Membership is never re-verified after the claim. A same-homeserver room the owner and agent are not both joined to passes.

Two things make this reachable rather than theoretical:

1. The new TS writers publish non-atomically. src/task-bridge.ts:243-266, 419-431 — forwardVoiceResultToOrigin, forwardVoiceResultToOwnerDm, forwardOfflineVoiceResult all writeFileSync(join(RESULT_DIR, file), …) directly into results/. CLAUDE.md requires temp + rename there precisely because the drain claims within a second. This PR adds three writers that violate it.
2. except Exception: pass claims anyway. Any throw out of the gate — including from the new membership verifier — delivers. That fail-open was reasonable when the gate only answered "whose file is this"; it is not once the gate carries a security verdict.

Fix needs both halves: temp+rename in those three writers, and re-run _voice_result_room_verified on the claimed body (or fail closed when the gate throws for a proactive-result-* with a [channel:] line).

---

Two further leads I did NOT verify — treat as unconfirmed

Reported by my first-pass scan; I ran out of scope before tracing them, so I am not asserting them:

• src/task-bridge.ts:1365-1371 — on the refused-origin path (resolveVoiceResultOrigin → null) it only console.logs, while onResult(result) has already fired without a deliveryNote, so the docked-room prompt may tell the model the answer was posted in the room when nothing was.
• src/task-bridge.ts:1192-1196 — [dm-only] is stripped before forwardOfflineVoiceResult's no-origin fallback writes an untagged proactive-result-*, which the gate deliberately exempts; a [channel: !room] + [dm-only] body could lose its guard.

If they hold, the second is the more serious (a privacy guard dropped on a fallback path).

Scope of this review, stated plainly

16 of 16 production files read; 4 of 12 test files read closely, 3 more entry-point-checked. Not reviewed: tests/ag2space-voice-{room,navigate}.test.ts (1175 lines), task-bridge-{format,isvoice}, voice-plugin-seam, voice-context-extra-lines, task-priority.test.py, and the markdown/fixture files. The three new Python suites do have runnable entry points, and gateway-voice-room-membership.test.py is a real negative integration against the production entry point with a positive control — worth saying, since a suite that runs nothing is the usual failure here.

Reviewed by Sutando core (qingyun-001).