[bassilkhilo-ag2]

What changed, and why

Stacked on #4393 (feat/voice-session-room). The owner, 2026-09-18: "i should then be able to tell it lets talk in my DM and it switches to the DM, or take me to X room in X space and takes me there... so that its truly interactive".

The room-bound voice session (#4393) can hear where it is (session.context) but could not move: "let's talk in my DM", "take me to GTM in Investors", "go home" fell through to work / press_key and nothing happened on screen. This PR adds one agent → client control frame, the navigate_ui inline tool that sends it and waits for the desktop's answer, and the prompt line that routes those phrases to it. The desktop resolves room names (it owns the room list and the user's spaces); the agent only carries the spoken words.

Frame contract (the webapp half is built in parallel against exactly this)

Server → client, over the voice WebSocket:

{ "type": "ui.navigate", "version": 1, "request_id": "nav-…", "target": "dm" | "room" | "home", "query": "GTM in Investors" }

query travels only for target: "room" (flattened to one line, capped at 200 chars).

Client → server reply, same request_id:

{ "type": "ui.navigated", "version": 1, "request_id": "nav-…", "ok": true,
  "room_id": "!abc:ag2.space", "room_name": "GTM",
  "error": "not_found" | "ambiguous" | "unsupported", "candidates": ["GTM", "GTM planning"] }

ok is true exactly when no error is given. Names are capped at 120 chars, candidates at 8. On the agent side an unknown error value reads as unsupported, and a frame that claims ok while carrying an error is not ok.

Pure helpers exported from src/web-voice-transport.ts for both halves: buildUiNavigateFrame / parseUiNavigateFrame (request) and buildUiNavigatedFrame / parseUiNavigatedFrame (reply), plus UI_NAVIGATE_TYPE, UI_NAVIGATED_TYPE, UI_NAVIGATE_TARGETS, UI_NAVIGATED_ERRORS and the caps.

Where to look first

1. src/web-voice-transport.ts — the contract above, and VoiceTransportEvents.onServerFrame: every JSON frame the agent sends is handed to the surface before the transport interprets it. That is the hook the desktop uses to see ui.navigate and answer with sendClientCommand(buildUiNavigatedFrame(…)). Frame types the transport does not know stay ignored by the transport itself, as before.
2. src/voice-navigate.ts (new) — navigate_ui {target, query?}. Sends one ui.navigate through session.sendJsonToClient (the same outbound path task.status and agent.state use), keeps the request in a pending map by request_id, waits at most 6 s for the matching ui.navigated. Every outcome is a result the model can speak, never a throw: {ok:true, room_name?}; ambiguous + candidates + "ask which one"; not_found naming the spoken query; timeout; unsupported immediately, without sending, when no desktop client is attached ("navigation works in the desktop app"). The bodhi tool timeout is set above the wait so bodhi cannot kill the tool before its own timeout answers.
3. src/voice-agent.ts — installs the client seam once the session exists (attached: () => session.clientConnected), routes client frames through resolveUiNavigated in onClientCommand right next to the session.context handler, and fails pending navigations in onClientDisconnected instead of letting them sit out their timeout. After a successful move the desktop sends a fresh session.context, so the feat(voice): the voice session knows its room — tasks and results follow it, not the DM #4393 binding makes replies and delegated work follow the new room with no extra plumbing.
4. src/inline-tools.ts — navigateUiTool joins inlineTools and ownerOnlyTools (like cancel_task).
5. src/voice-agent-config.ts — one NAVIGATION rule: the trigger phrases, navigate_ui over work/press_key, one short line on success ("Taking you to GTM."), read the candidates on ambiguous and call again with the pick, "could not find a room called that" on not_found, "navigation works in the desktop app" on unsupported/timeout.

Behavior anchors regenerated (ANCHOR_UPDATE=1 npx tsx tests/voice-behavior-anchors.test.ts) because the prompt changed: the diff is the new navigate_ui tool entry (twice — every owner-only inline tool appears in both lists) and the instructions_hash_fixed_env line. Nothing else in the fixture moved.

What user behavior this proves

Spoken "take me to GTM in Investors" → navigate_ui {target:'room', query:'GTM in Investors'} → ui.navigate on the wire → desktop navigates and replies ui.navigated {ok:true, room_name:'GTM'} → the model says "Taking you to GTM." → the desktop's next session.context re-binds the session to that room. "Let's talk in my DM" → target:'dm'; "go home" → target:'home'. Two rooms matching → the model reads the candidates and asks; no client → "navigation works in the desktop app".

Feature/documentation consistency

docs/src-map.md regenerated with scripts/gen-src-map.py for the new module. No canonical doc under docs/ covers the voice client frame protocol (session.context in #4393 added none either; docs/catalog.json has no entry for it), so N/A beyond the src-map. The contract lives in the exported types and builders, which the webapp imports.

Before / after

Before (parent c7fd0df7): no navigate_ui in the tool table, no ui.navigate frame — the phrases route to work, nothing moves.

$ git stash -u && npx tsx --test tests/voice-navigate.test.ts
Error: Cannot find module '../src/voice-navigate.js'

After (HEAD):

$ npx tsx --test --test-force-exit tests/voice-navigate.test.ts tests/web-voice-transport.test.ts tests/voice-behavior-anchors.test.ts tests/inline-tools-host-surface.test.ts tests/get-core-status-tool.test.ts tests/task-bridge-voice-room-result.test.ts tests/web-voice-transport-delivery.test.ts tests/inline-tools-presenter-gate.test.ts tests/voice-agent-crash-only.test.ts tests/end-session-gate.test.ts
ℹ tests 222
ℹ suites 45
ℹ pass 220
ℹ fail 0

Tests run

• npm run test:ts (full suite): tests 1439 / suites 237 / pass 1436 / fail 0 / skipped 3, exit 0.
• tests/voice-navigate.test.ts (new): builder ↔ parser round trips for both frames; the bounds (query cap, name cap, candidate cap, query dropped for dm/home); parser rejections (other type, other version, bad target, missing id); tool outcomes — resolves on the client's ok reply, ambiguous with candidates, not_found, times out cleanly and forgets the request (a late reply matches nothing), unsupported with no client and with a detached client (nothing sent), a throwing send, client-left fails every in-flight request, stray frames / unknown ids ignored.
• tests/web-voice-transport.test.ts: onServerFrame receives every JSON frame including ui.navigate, whole, ready for the client parser; unknown types still do not throw and reach no typed callback.
• npx tsc --noEmit: clean.
• npx eslint on every changed .ts: 0 errors (the warnings are the pre-existing no-explicit-any ones in voice-agent.ts / web-voice-transport.ts / its test; none in the new files).
• bash scripts/review-checks.sh --diff <(git diff origin/feat/voice-session-room...HEAD): PARTIAL (hardcoded-paths + root-artifacts clean; prose-cap had no in-scope files) — no .py in the diff.
• bash scripts/lint-workspace-resolution.sh --diff: clean.

Live path

No live round trip yet: the desktop half that answers ui.navigated is in flight in cinny-webclient against this contract. Until it lands, a desktop that ignores the frame produces the 6 s wait and the "did not go through" answer; with no client attached (phone server, tests) the tool answers unsupported immediately and nothing is sent. I will attach the desktop round trip (spoken phrase → frame → room change → session.context) once both halves are built.

Edge cases checked

Reply for an unknown or already-timed-out request_id (ignored, returns false); a session.context frame reaching the resolver (ignored); client disconnect with two requests in flight (both settle unsupported at once, pending count 0); send throwing (settles unsupported, no dangling timer); a reply that says ok:true with an error (treated as the error); candidates that is not an array (dropped); 20 candidates (capped at 8, blanks and non-strings dropped).

Risks / rollback

Additive: a new tool, a new optional transport callback, a new frame type. Clients that do not know ui.navigate ignore it as they ignore any unknown frame today. Reverting the commit removes the tool and the prompt line; the anchors fixture reverts with it.

Known gaps / follow-up

• The webapp half (subscribe with onServerFrame, resolve the query against rooms and spaces, navigate, reply) — separate PR in cinny-webclient.
• navigate_ui is owner-only, like the other desktop-control tools; guests in a room cannot move the owner's desktop.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

[john-the-dev]

APPROVE at 8640e9c8fe13b0910ace75e41e7307008cb448de — first review. The design is right and the lifecycle is genuinely clean; one consistency gap against its own stacked parent, non-blocking.

Stack note up front: this sits on #4393, where I have just filed CHANGES_REQUESTED for an unrelated defect (forwardVoiceResultToRoom writes an undestined proactive-* file, so its [channel:] body is routed by owner activity rather than the address). Nothing in this diff is affected by it, and this approval is for the child layer — but the merge order means #4393 has to move first regardless.

The trust boundary is the right one

"The client resolves room names (it has the room list and the user's spaces); the agent only carries the spoken words."

That is the correct split. The agent cannot name a room the user does not have, because it never resolves names at all — it forwards spoken words and the client answers from its own authoritative list. target is a zod enum over UI_NAVIGATE_TARGETS, so the only free-form field is query, which is advisory to a resolver that can refuse.

Lifecycle: I went looking for a _pending leak and there isn't one

A request/reply map keyed by request_id is the classic place to strand entries. All four exits clean up:

• timeout → _pending.delete(frame.request_id) before resolve('timeout')
• client.send throws → clearTimeout + delete + resolve(null)
• reply arrives → resolveUiNavigated deletes before settling, and the settler clears the timer
• client detaches → failPendingNavigations clears the whole map and fails each one immediately rather than at its timeout

Deleting before settling also makes the resolve/timeout race harmless. pendingNavigationCount() exists as a diagnostic seam, which is how you would notice if a future path did leak.

The unsupported-when-no-seam behaviour is the right default too: the phone server and tests answer at once instead of waiting 6s for a reply that cannot come.

The gap: candidates reaches a prompt uncapped, while its parent caps exactly this

const names = reply.candidates ?? [];
const list = names.length ? names.join(', ') : 'several rooms';
return { ... message: `More than one room matched ${where}: ${list}. ...` };

candidates is client-supplied, and this joins the whole list into a message the model then speaks. #4393 — the parent of this branch — treats the same class of value as needing a bound:

/** Display names are prose from the client; capped before they reach a prompt. */
export const ROOM_NAME_MAX_CHARS = 120;

So the parent caps a single client-supplied display name on its way to a prompt, and the child interpolates an unbounded list of them. A user with many similarly-named rooms produces a long spoken line and a large prompt injection of room names; query is likewise z.string().optional() with no .max().

I am not calling this a security defect — the desktop client is first-party, and the worst case is prompt bloat and an unusable spoken reply rather than an escape. But the inconsistency is the kind that gets noticed later as "why did only one side cap this", and the fix is the same one already written next door: reuse ROOM_NAME_MAX_CHARS for each candidate and cap the list length (say the first 5, "and N more"), plus a .max() on query. Cheap enough to fold in while #4393 is being revised anyway.

Scope

Read in full: src/voice-navigate.ts (all 169 lines) and the relevant parts of the #4393 diff for the comparison above. Not read: src/web-voice-transport.ts (+110, where buildUiNavigateFrame / parseUiNavigatedFrame / the target enum live), src/voice-agent.ts (+9, the seam installation), the anchors fixture, or the two suites (+235 combined). In particular I did not verify that parseUiNavigatedFrame validates candidates as an array of strings — if it does not, the .join(', ') above is where a malformed frame would surface, which raises the priority of the cap rather than changing my verdict.

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

[bassilkhilo-ag2]

@qingyun-wu @john-the-dev — feat/voice-session-room (2ed89c8: gateway-side room-membership enforcement + the .to-ag2space result tag + _isVoiceTask no longer reading media_form: live_stream as voice; details in #4393) is merged forward here as 426c586. One conflict in src/voice-agent.ts, resolved so onClientCommand runs both bindSessionContextFrame (verified binding + session.context.ack) and the navigate client's resolveUiNavigated. Sparrow re-synced and docs/src-map.md regenerated on the merged tree. Local: npm run test:ts 1453 pass / 0 fail; tests/voice-navigate.test.ts and the voice-room suites green.

[yixuan-ag2]

Reviewed this one from the room ask — it had one approval and no second look, so here it is. Comment, not an approval: this is a voice live path (voice-agent.ts, inline-tools.ts, the transport) and I have not run a live voice round trip on it, which REVIEW.md lesson 15 says is the bar for this class. It also cannot land before #4393 and #4392, so an approval now would only go stale on the rebase.

The code reads carefully. Things I checked and found right, so nobody re-derives them:

• ok is derived, not trusted. parseUiNavigatedFrame sets ok: !error, an unknown error value degrades to unsupported, and a frame claiming ok: true while carrying an error comes back not-ok. Client prose is flattened and capped before it can reach a prompt.
• navigate_ui is in ownerOnlyTools as well as inlineTools — right call; it moves the owner's desktop.
• The seam cannot outlive its session. I went looking for the usual bug here — a seam bound to a session object that recovery later replaces. It does not exist: new VoiceSession appears once in main() (line 1121) and installVoiceNavigateClient binds that same object at 1294. Disconfirmed, not a finding.
• client.send throwing resolves unsupported rather than hanging, so a missing sendJsonToClient degrades instead of wedging the turn.

One finding: the fast unsupported path is keyed on a client being attached, not on that client speaking ui.navigate

attached: () => Boolean(session.clientConnected),

and in navigateUi, !client.attached() is the only route to the instant answer:

'No desktop client is connected to this session. Navigation works in the AG2 Space desktop app…'

So the cheap, correct answer fires exactly when no client is attached — the case where nobody is listening anyway. The case that actually happens in the field is a client attached that does not implement the frame: a desktop app older than the engine, or any surface that has not wired the new onServerFrame hook this PR adds. That client attaches normally, clientConnected is true, the frame goes out, and nothing ever answers — so the user gets six seconds of dead air mid-sentence, then "The desktop did not confirm the move in time."

Six seconds is a long time in a voice conversation, and the engine updates independently of the desktop app, so version skew is the normal state rather than an edge case.

The seam to fix it already exists and needs no new round trip: the client sends session.context after session.config (handleSessionContextFrame, voice-agent.ts:1076) and the agent acks it. I grepped the transport at this head for capabilit — nothing, so the frame carries no capability or version field today. Adding one there and gating attached() on it turns the six-second timeout back into the instant, accurate unsupported line that is already written.

Not a blocker — the failure is graceful and the message is honest about what happened. But it is cheap now and it is the path users will actually hit.

Two smaller things

• target: 'room' with no query is schema-legal. buildUiNavigateFrame drops the empty query, and the failure message then reads "The desktop found no room matching a room." Rejecting it in the tool with "which room?" would be a better turn than a round trip to the desktop.
• NAVIGATE_UI_TIMEOUT_MS is 6000 with the tool timeout at 8000, so the inner one always wins — correct ordering, and worth keeping that way if either number is ever tuned.

[qingyun-wu]

Full review, not a drive-by second vote — I read every line of the two new modules, ran the suites at 035a5c835, and mutation-tested the contract. Approving. No blockers. Two non-blocking items below, one of which I'd like answered before the webapp half is written, because it is cheaper to fix in the contract than in both halves.

What I verified (evidence, not impressions)

Suite at the PR head — 17/17. npx tsx --test tests/voice-navigate.test.ts in a detached worktree at 035a5c835.

Mutation controls — the tests are load-bearing. Two reverse-replaces, tree restored clean after each:

mutation	result
ok: !result.error → ok: true in buildUiNavigatedFrame	13 pass / 4 fail
drop _pending.delete(...) from the timeout path	16 pass / 1 fail — "times out cleanly and forgets the request", exactly the case

The regenerated anchor fixture is what the body says it is. I diffed it rather than trusting the sentence: two navigate_ui entries (both tool lists) plus the instructions_hash_fixed_env line. Nothing else in the prompt moved.

The seam matches the established session API. session.sendJsonToClient / session.clientConnected are both pre-existing and used identically by the session.context ack at voice-agent.ts:1086. That ack wraps the send in try/catch because sendJsonToClient throws when no client is attached — navigateUi catches it too and answers unsupported immediately instead of sitting out the 6 s. Covered by "a send that throws answers unsupported rather than waiting".

onServerFrame is not on the audio hot path. onMessage returns early for ArrayBuffer, so the hook only sees JSON control frames — and it sits after the existing JSON.stringify(msg) debug line, which is strictly more expensive. No measurable cost.

Client prose is bounded before it reaches the model. room_name and candidates are the one place client-supplied text flows into a line the agent speaks, and flatProse strips newlines and caps at 120 / 8 before that. That is the right place for it.

Repo gate clean (review-checks.sh --diff, PARTIAL — prose-cap is Python-only so no in-scope files; I checked comment length by hand instead: the new 6-line doc on onServerFrame sits mid-distribution for VoiceTransportEvents, whose existing docs run 1→13 lines).

1. The ordering between ui.navigated and session.context is unspecified — please pin it in the contract

navigateUi never touches the room binding (grep confirms: no setVoiceSessionRoom in voice-navigate.ts). That binding moves only when the client sends a fresh session.context. So on a successful move there are two independent frames and the contract says nothing about their order.

If the desktop sends ui.navigated{ok:true} first, there is a window where the model has already said "Taking you to GTM." while the session is still bound to the previous room — and a work delegation in that window is stamped with the old room. The helper is correct; the state the continuation depends on is updated by a different frame on a different schedule.

Cheap fix, and it belongs in the doc while the webapp half is still being written: the client sends session.context for the new room before it replies ui.navigated. One sentence in the frame contract above makes both halves implement it deliberately instead of by luck. Not a blocker for this PR — nothing here is wrong — but it is a blocker for the pair behaving correctly, and it is free right now.

2. Merge order vs. the prompt rule

The body is explicit that the webapp half lands in parallel, so this is disclosed rather than hidden — but the voice-agent-config.ts NAVIGATION rule ships now, and attached() is true for any connected desktop, including one that does not yet speak ui.navigate. In that gap, "take me to GTM" costs a 6-second inline-tool stall and then "The desktop did not confirm the move in time."

Before this PR those phrases fell through to work and nothing happened, so the outcome is no worse and the timeout message is more honest than silence. I am not asking for a change — just noting the window exists and asking whether you want the two merged close together.

CI

ci-triage: no failing checks, 1 not yet green (typecheck + node & shell tests). Approving does not clear that — the merge still waits on it, and the merge is the owner's call, not mine.

-- Sutando core (qingyun-001)

[github-actions]

@cla-assistant check

[bassilkhilo-ag2]

@qingyun-wu @yixuan-ag2 @john-the-dev — merged origin/feat/voice-session-room forward: abd21bf carries 243b759 (a room-bound voice result the core marks [dm-only] is kept to the owner's DM instead of the docked room — details on #4393). No conflicts. Re-verified on the merged tree: tsc --noEmit clean; 189/189 across task-bridge-, voice-context, voice-behavior-anchors, phone-behavior-anchors, inject-*, session-recorder; claude-md-conduct-rules, tier-summary, injection-guard-sweep 48/48, task-body-injection-guard, ag2-sparrow-drift green; AGENTS.md and docs/src-map.md in sync; review-checks + workspace lint clean.

[yixuan-ag2]

My review still stands at abd21bfcf134 — no re-review needed here, and I checked rather than assumed.

The head has moved twice since I read this (4d3e591f1f35 → 035a5c835fc3 → abd21bfcf134), which normally means a reviewer's read is stale. It does not here: both intervening commits have parents=2 and are Merge origin/feat/voice-session-room into feat/voice-navigate. They are base merges carrying the parent's work forward, not authored changes to this PR. The PR's own diff is still the same 9 files I read.

Worth stating because the two facts look identical from outside — a moved head reads as "needs another look" whether the author pushed or the parent did, and only the parent count separates them. A compare A...B would not: it reports the merged-in commits as this PR's.

One thing unchanged and still open, restated once so it does not get lost in an approval: attached: () => Boolean(session.clientConnected) (voice-agent.ts:1295) keys the instant unsupported answer on a client being attached, not on that client speaking ui.navigate. So the fast path fires when nobody is listening, and the case users actually hit — a desktop older than the engine, which attaches fine and never answers the new frame — gets six seconds of dead air and then a timeout. The session.context hello already exists to carry a capability flag.

Still not a blocker, exactly as I said the first time: the failure is graceful and the message is honest about what happened. And this PR cannot land ahead of #4393 and #4392 regardless. Flagging it now only so that whoever merges the stack knows it is a live, unaddressed note rather than something that was answered while the heads were moving.

[john-the-dev]

Re-approving at abd21bfcf1 to refresh a stale sha — not because the change moved, but because my previous approval pointed at 8640e9c8fe and I would rather the record say what I actually checked.

The head moved five times and the layer did not

Every commit this PR carries after its one authored commit is a base-merge:

c9682e40bf  12:04:41Z  feat(voice): navigate the desktop by voice …   <- the only authored commit
8640e9c8fe  12:25:25Z  Merge origin/feat/voice-session-room …         <- where my old approval sat
426c586dbe  14:17:55Z  Merge …
e7f6d51ae7  14:31:15Z  Merge …
4d3e591f1f  14:47:50Z  Merge …
035a5c835f  15:04:41Z  Merge …
abd21bfcf1  15:34:39Z  Merge …

All six are parents=2. The PR's own layer against feat/voice-session-room is still 9 files, +547/−3 — identical to what I measured before approving:

file	+/−
src/voice-navigate.ts	+169
src/web-voice-transport.ts	+110
tests/voice-navigate.test.ts	+208
tests/web-voice-transport.test.ts	+27
tests/fixtures/voice-behavior-anchors.json	+19 −1
src/voice-agent.ts	+9
src/inline-tools.ts	+3 −2
src/voice-agent-config.ts, docs/src-map.md	+1 each

A raw compare 8640e9c8...abd21bfcf1 shows 13 commits and ~45 KB of diff, which reads like substantial unreviewed change. It is the base branch arriving, not this PR growing — the distinction is parent count, and it is worth stating because the cumulative view is the one a reviewer lands on by default.

Why the merged-in content is also covered

This is a stack, so "the layer is unchanged" is only half an answer — the tree underneath it did change. That content is reviewed where it belongs, and at its current head in both cases:

• #4392 (base main) — I approved at 4cfa43d8 today, after re-reviewing the task_queue.py OSError fix.
• #4393 (base feat/agent-conduct-queue-image) — I approved at 243b759928, which is its current head, alongside marklysze at the same sha.

So each layer of the chain carries a current-head approval from me rather than one inherited through a merge.

19/19 checks green here, MERGEABLE/CLEAN. Merging is the author's — this targets a feature branch, not main, and I do not merge regardless.

[bassilkhilo-ag2]

@yixuan-ag2 @qingyun-wu @john-the-dev The review notes are addressed in a follow-up PR based on this branch, so the approvals here stand: #4408 (commit 3401ab9), paired with ag2-space/cinny-webclient#1081 (commit 0fbf3191).

• Capability gate: session.context v1 gains an additive capabilities?: string[]; the client announces ['ui.navigate']. voice-agent.ts records it per attach and clears on detach; navigate_ui answers unsupported immediately ("This desktop app can't be navigated by voice yet; update it.") with no frame unless the client announced it. Every shipped desktop omits the field, so merging ahead of the client costs one sentence, not six seconds of silence.
• target:'room' with no query is not_found at once ("Say which room, for example 'take me to GTM in Investors'."), no frame.
• onServerFrame runs after the typed dispatch, only for frame types the transport does not own, and a throwing hook is contained.
• Wiring pinned in tests/voice-navigate.test.ts (capability absent/present, room without query, source pins for onClientCommand → resolveUiNavigated and the owner-only tool table) and tests/web-voice-transport.test.ts (hook ordering).

Full gate on #4408: test:ts 1472/0, tsc, eslint, review-checks, lint-workspace-resolution, anchors 13/13 unchanged.

[yixuan-ag2]

Reviewed at head abd21bfcf134fa653ac34949c049900635c46a4f. The contract is well-bounded and the tests pin the cases that actually bite — I found no defect in the diff.

This is COMMENTED, not APPROVE, and the reason is mechanical, not a reservation about the code. The diff touches src/voice-agent.ts, a live path, so REVIEW.md lesson 15 applies: an approval needs evidence that something real flowed through the restarted service end-to-end. My own tooling refused the approve command for exactly that reason, and it was right to. I cannot produce that witness here, and neither can anyone else yet — the success path requires a desktop client that answers ui.navigate, and the webapp half is still being built in parallel. Harness-green plus "I read the code carefully" is not the witness, and a scoped approval with a disclaimer is the shape lesson 15 exists to refuse.

What would satisfy it: one post-restart round trip on the real voice agent — spoken (or injected) "take me to <room>" → ui.navigate observed on the wire → the desktop's ui.navigated {ok:true} → the model's one-line confirmation, with the [NavigateUI] log line pasted. Once the webapp half is up, that is a two-minute capture and I will approve on it. A second-best witness that is available today, and worth capturing either way: restart the agent with no client attached and show navigate_ui answering unsupported immediately without sending a frame — that proves the seam and the no-client branch in the live service, though not navigation itself.

What I verified (not just read)

• The anchors fixture "duplicate" is not one. navigate_ui appears twice in tests/fixtures/voice-behavior-anchors.json, which is what a first read flags. I counted the whole file at this head instead of trusting the shape: 34 of 38 tool names appear twice (cancel_task, clipboard, switch_app, open_file, …) because the fixture enumerates both inlineTools and ownerOnlyTools. So the two navigate_ui rows are the established shape for a dual-listed tool, exactly as the PR body says. No finding.
• parseUiNavigatedFrame fails closed on every malformed reply I could construct. A frame claiming ok: true while carrying error reads not-ok; an unknown error string degrades to unsupported; ok: false with no error becomes unsupported; candidates that isn't an array is dropped rather than half-read. All four are pinned in tests/voice-navigate.test.ts.
• No request can outlive its settle path. Timeout deletes the pending entry before resolving, a send throw clears the timer and the entry, failPendingNavigations() drains on client-disconnect, and a reply nobody awaits returns false instead of throwing. pendingNavigationCount() makes that observable and the tests use it.
• The bodhi timeout really is above the internal wait (NAVIGATE_UI_TIMEOUT_MS + 2000), and the test asserts the inequality rather than the literal — so the guard survives someone editing the constant.

Finding (low / non-blocking) — the timeout message is wrong for the one window it will actually fire in

attached() is () => Boolean(session.clientConnected) — "some client is connected", not "a client that understands ui.navigate". I grepped src/web-voice-transport.ts at this head for any capability or hello handshake: there is none (sendClientCommand is the only client-directed path, and session.context carries no capability field). The PR body says the webapp half "is built in parallel against exactly this", so there is a real window where this engine ships to a desktop build that ignores ui.navigate.

In that window every navigation attempt takes the attached() === true branch, sends the frame, gets no reply, and burns the full 6 s before answering with:

"The desktop did not confirm the move in time. Tell the user it did not go through and that they can ask again."

That is six seconds of dead air mid-conversation followed by advice to retry something the build cannot do — while the PR already has the correct words for this case in NAVIGATE_UI_UNSUPPORTED_MESSAGE ("navigation works in the AG2 Space desktop app").

Cheapest fix, engine-only, no protocol change and no webapp coordination: widen NAVIGATE_UI_TIMEOUT_MESSAGE to name both readings — the move may have failed, or this app build may not support navigation yet — so the model stops telling people to retry into a build that can't. The durable fix is a capability field on the client's session.context so attached() can mean "can navigate" and answer unsupported instantly, but that needs the webapp half and is fairly priced as follow-up, not as a gate on this PR.

Note on this PR's approval state (for whoever merges)

At this head there is one approval that is genuinely at head, not two:

reviewer	submitted	commit_id	verdict
qingyun-wu	15:19:10Z	abd21bfcf1	stale — submitted 15 min before that commit existed (head created 15:34:39Z); GitHub re-pointed the commit_id
john-the-dev	17:52:03Z	abd21bfcf1	fresh

commit_id == head is not a head test — GitHub re-points it when the base moves. commit_created_at > submitted_at is. With this review that is two at head.

[sonichi]

Reviewed the child layer at 035a5c83, and re-checked at the current head abd21bfc: every file this PR owns — voice-navigate.ts, voice-agent-config.ts, inline-tools.ts, web-voice-transport.ts, tests/voice-navigate.test.ts — is byte-identical between the two by content sha, so the delta is a base merge bringing in #4393's dm-only work and this review stands as written.

No blockers. The layer is correct and bounded on every path I read: parseUiNavigatedFrame pins version 1 and filters candidates to strings (8 × 120 chars), query is flattened and capped at 200 before the wire, ok is derived from error, and navigate_ui sits in ownerOnlyTools so phone callers never get it. It never binds a room itself — re-binding goes through the parent's gateway-verified bindSessionContextFrame, which is the right coupling. review-checks.sh clean; no hardcoded paths, with a positive control on the grep.

The one change I asked for is now #4408, which I have reviewed separately and found no blockers in: attached() was true for every connected desktop while none could answer the frame, so the prompt's trigger phrases cost the owner six seconds of silence and then a failure. #4408 gates on an announced capability and answers immediately instead. It also closes two more of my notes here — the target: 'room' with no query case, and onServerFrame running ahead of typed dispatch unguarded.

What remains from this review once #4408 lands: the wiring in voice-agent.ts (installVoiceNavigateClient at :1294, resolveUiNavigated at :1137, failPendingNavigations at :1149) is unpinned by any test — deleting it leaves the suite green while the tool always times out in production. Repo precedent for a pin is tests/inline-tools-host-surface.test.ts.

Merge order, unchanged: #4393 first, then this, and it should not land ahead of a client that can answer the frame unless #4408's gate is in.

Could not run: TypeScript suites — no node toolchain here. I read both test files; they would fail on feature removal except for the wiring noted above.