Repository navigation
fix(task-bridge): bind a voice task to the origin and words it was asked from, not the session it moved to - #5159
Conversation
…n the session moves during the spoken-turn wait
The work tool waits up to SPOKEN_WAIT_MS for the owner's late transcription
(`await _awaitSpokenTurns(2)`), and only then read `_voiceSessionOrigin`.
`setVoiceSessionOrigin` replaces that module-level value whenever the live
session moves to another origin. A move inside the wait therefore bound a
task asked from origin A to origin B: its header named B and its result was
routed to B.
The origin is now read synchronously, next to the dedup reservation, which
sits before the first await for the same reason.
Test: tests/task-bridge-voice-origin.test.ts. The new case opens the wait
(owner spoke just now, no utterance landed), moves the session 300 ms in,
and asserts that the task and its header keep the first origin. It fails on
the parent ("actual: 'place-b', expected: 'place-a'") and passes here.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE at 9a9d94e14dc0a0d0488b51d7edcd89040a21bfe1.
The fix is correct and minimal. const origin = _voiceSessionOrigin now runs synchronously right after the dedup reservation (src/task-bridge.ts:935), before the await _awaitSpokenTurns(2) at :955 (up to SPOKEN_WAIT_MS = 1500, :297). The header, _rememberTaskOrigin and the guidance line all use that one captured value.
What I verified
- Falsifying check: I ran the new test at this head and got 15/15 pass. Then I checked out only
src/task-bridge.tsfrom the parent54d6465fand kept the new test. It failed 14/1 withactual: 'place-b', expected: 'place-a'. The test genuinely pins the move-during-wait interleaving through the realworkTool.execute. - CI: the three red jobs are inherited from main. At this head,
python standalone tests (1/5)fails onlytests/agy-task-notifier.test.py.(3/5)failsagy-notifier-dependency-gate.test.pyandtmux-session-targets-are-exact.test.py.tsc + tests (clean install)is the aggregate gate failing because the python job failed. All of these are a subset of what fails on main at the base (run 37484638091: the same files, plusclaude-task-notifier-submit.test.py). The PR touches no.py. (The body lists two failing files; 3/5 also failstmux-session-targets-are-exact.test.py, also inherited.) scripts/review-checks.shongh pr diff 5159: hardcoded-paths and root-artifacts are clean. PARTIAL only because there is no.pyfile.
Not a blocker, but coordinate before merge: #5035 overlaps this (same author, open since 2026-10-02, no reviews). #5035 makes the same change in a broader form. It captures the origin, its headers and recent conversation at execute entry, and fences _awaitSpokenTurns to a context revision so a moved session's words are not attached. The two PRs will conflict on these lines. One should be closed, or #5035 rebased to supersede this one. Merging both as-is is not possible.
Nits (non-blocking)
- The
setVoiceSessionOrigindocstring (:225-226) still says tasks "carry the origin current at write time". After this change they carry the origin current at the reservation. #5035 rewords it. - The read is not at the true start of the call. On non-Windows,
await import('node:child_process')at:891(watcher probe), and on the concat fast path at:858, still yields before:935. The window is a tick, not 1.5 s, so it is practically negligible. Still, "asked from" strictly means call entry, which is where #5035 reads it. - Residual not fixed here:
_awaitSpokenTurnsandgetRecentConversation(4)still read the live session after the wait. A move followed by a new utterance within 1.5 s could attach words spoken in B to a task now addressed to A. Before, the task was wrong but consistent (all B). Now it is A-addressed with possibly B words. Narrow, and #5035 addresses it.
Not verified: no live voice round trip (the body says so too; the interleaving is hard to stage live). I did not run the full npm run test:ts.
…ot after its awaits Review of the first version found two more paths by which a task could take the origin or the words of a session that moved while the call was in flight: - workTool.execute already awaited before the reservation, in the watcher probe's dynamic imports and the concat fast path. A switch made right after the call still bound the new origin. The origin is now read at entry, before any await: an async function runs synchronously up to its first await. - The spoken-turn wait kept polling the live turns provider, and the recent conversation was read after the wait. Both could carry the new session's words into a task addressed to the old one. setVoiceSessionOrigin now bumps a generation whenever the origin changes. _awaitSpokenTurns takes an isCurrent predicate and discards anything read once the session has moved, re-checking after every read because the move can happen during the read. The recent conversation is snapshotted at entry. The tests are now deterministic, with no timers: - The provider moves the session on its first read after the call began, so the move lands inside the wait by construction. - A second case moves the session immediately after the call returns. On the parent both fail (actual 'place-b', expected 'place-a'). Here, 16/16 pass. Each part of the fix was reverted on its own (context read after the wait, unbound wait, no re-check after a read, origin read after the earlier awaits), and each revert fails a named assertion. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
akag2
left a comment
There was a problem hiding this comment.
APPROVE at 8125142135b7152692ab70e70ad1744088ee02a1. Ready to merge once required CI finishes at this head and #5035 is closed or rebased. Most of #5035 is now covered by this PR.
The new commit 81251421 captures three things synchronously at execute entry, before any await: the origin, an origin generation, and getRecentConversation(4). It also fences _awaitSpokenTurns with isCurrent(), so turns read after a session move are dropped instead of being attached to the task. That closes both earlier nits: the gap before the first await, and the case where the session moves and a new utterance arrives during the wait. The docstring is fixed too.
Verified
- The tests fail before the fix and pass after. At parent
54d6465f, both origin tests fail withactual: 'place-b', expected: 'place-a'(14 pass, 2 fail). At this head, the origin, spoken-block and format suites pass 36/36. - The move-then-new-utterance nit is reproduced and closed. At the previous head
9a9d94e1, the new test fails on "none of the new session's words reach the task". - The test covers both halves of the fix. Removing only the
isCurrentfence makes it fail. Restoring the post-await transcript read makes it fail on the same assertion. - No regression in the ordinary case.
voice-agent.tswritesconversation.logonly onturn.end, after the tool returns, so reading it before the wait sees the same lines as reading it after. Verified by reading the code, not by a live run. - Full suite:
npm run test:tsgives 1640 tests, 1637 pass, 0 fail. - No hardcoded paths.
scripts/review-checks.shis clean on this diff, and a planted/Users/alice/...control still trips it.
Nits (non-blocking)
getRecentConversation(4)now also runs on the screen-redirect and dedup early-return paths, which don't use it.- If the session goes A→B→A, or is released, during the wait, the spoken block is dropped even though the origin matches again. That's the safe choice; noting it so it isn't mistaken for a bug later.
Not verified: a live voice round trip after restart (one ordinary task-in, result-back is still worth doing), and CI at this head, which was still running.
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE at 8125142135b7152692ab70e70ad1744088ee02a1. This reviews the delta since my approval at 9a9d94e1: one authored commit, 81251421 (single parent).
The delta closes all three nits from my previous review
- The origin is read at call entry.
origin,originGenerationandrecentAtAskare now read at the top ofworkTool.execute(src/task-bridge.ts, right after the arg destructure), before the watcher-probe imports and the concat fast path. Before, they were read after those awaits. - The wait is bound to the session.
setVoiceSessionOriginbumps_voiceOriginGenerationwhen the channel or target changes, and_awaitSpokenTurnstakes anisCurrentpredicate. It is checked after every read, including the first, and returns[]once the session has moved. All other callers keep the default() => true. Passingundefinedforsleeptakes the default. - Recent conversation comes from ask time. The recent-conversation block now uses the entry-time read, not a read after the wait.
- The docstring at
setVoiceSessionOriginis corrected.
Falsifying check, run locally
- At this head,
task-bridge-voice-origin+spoken-block+format: 36/36 pass. - Restoring only
src/task-bridge.tsfrom9a9d94e1and keeping the new tests gives 14 pass / 2 fail. One failure hasplace-bwords in the spoken and recent blocks of aplace-atask; the other hasactual: 'place-b'. So the new tests pin exactly the residual I flagged.
CI on this head (run 37516658155, attempt 1): three jobs fail, all of them inherited from main, none caused by this PR.
python standalone tests (1/5)failstests/agy-task-notifier.test.py(2 failures).(3/5)failstests/agy-notifier-dependency-gate.test.py(2) andtests/tmux-session-targets-are-exact.test.py(1).tsc + tests (clean install)is the aggregate of those.
This is the same set that fails on main at base 54d6465f (run 37484638091). #5161, still open, fixes main. This PR touches no .py, and its diff contains no tmux string.
scripts/review-checks.sh on gh pr diff 5159: hardcoded-paths and root-artifacts are clean. The result is PARTIAL only because the diff has no .py file.
Coordinate before merge: #5035 (same author, still open) overlaps these lines. This PR now does what #5035 did here, so close #5035 or rebase it onto this one.
Not verified: a live voice round trip, and the full npm run test:ts.
|
@cla-assistant check |
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
sonichi
left a comment
There was a problem hiding this comment.
Reviewed as a src/ code owner -- this is the piece actually missing: john-the-dev and akag2 have both approved, but neither is listed in CODEOWNERS for src/ (@sonichi @qingyun-wu), so require_code_owner_review was never satisfied regardless of count.
Content: captures _voiceSessionOrigin, _voiceOriginGeneration, and getRecentConversation(4) atomically before the first await in workTool -- the exact race window (session moves while the call is in flight) is closed by binding before anything can yield. _awaitSpokenTurns now takes an isCurrent() callback gated on generation equality, so a session that moves mid-wait returns [] rather than leaking the new session's words into the old task. Two new regression tests cover both race shapes (mid-wait move, immediate move-right-after-call) and both assert the task keeps the origin and words it was actually asked with. CI green, 0 failures.
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed at c396182.
Requested by yixuan-ag2. Fresh detached worktree of the head in a scratch dir; npm ci at the head (282 packages, exit 0); main for comparison is f7b69fc81ce8f724ade258dd997457809d98aa86 (the merge-base).
Verdict
No blockers. The fix is where the policy lives (workTool.execute and _awaitSpokenTurns in src/task-bridge.ts), it is small, and the two new deterministic cases fail on main and pass at the head for the reason the body states.
What I checked
The three read-after-await paths are closed.
src/task-bridge.ts:852-855reads_voiceSessionOrigin,_voiceOriginGenerationandgetRecentConversation(4)before the firstawaitinexecute()(the first awaits are theawait import(...)fast paths at 872/892/905 and the concat path at 918-927). An async function runs synchronously to its first await, so a move cannot land between these three reads.src/task-bridge.ts:967passes() => _voiceOriginGeneration === originGenerationinto_awaitSpokenTurns;read()at 321 re-checks after every_spokenTurnsread, including the first one, and returns[]on a move, so no post-move words reachspokenBlock.src/task-bridge.ts:977usesrecentAtAskinstead of a freshgetRecentConversation(4).src/task-bridge.ts:988-990bind and write the header from the capturedorigin;grep -n "_voiceSessionOrigin" src/task-bridge.tsshows no other read insideexecute()(only the declaration at 221, the setter at 231-232 andgetVoiceSessionOrigin()at 335).setVoiceSessionOrigin(229-233) bumps the generation on any channel or target change, including bind -> release -> rebind of the same origin (two bumps), and not on null -> null. Conservative in the right direction.- Other callers of
_awaitSpokenTurns(tests/task-bridge-spoken-block.test.ts:76,81,112) keep the defaultisCurrent = () => true, so their behaviour is unchanged.
Tests, run here.
Head, the three files the body names:
$ npx tsx --test --test-force-exit tests/task-bridge-voice-origin.test.ts tests/task-bridge-spoken-block.test.ts tests/task-bridge-format.test.ts
ℹ tests 36
ℹ pass 36
ℹ fail 0
exit 0
The head's tests/task-bridge-voice-origin.test.ts copied onto the main tree (same package-lock.json, same node_modules):
ℹ tests 16
ℹ pass 14
ℹ fail 2
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
actual: 'place-b',
expected: 'place-a',
(both new cases)
exit 1
That is the failure the body reports at the parent, reproduced independently.
Test design. The move in the first case is triggered from inside the turns provider on its first read after armed = true, and armed is set after delegate() returns its promise, so the move lands inside the spoken-turn wait by construction. No timers, no sleeps; both cases ran in well under a second here.
Hardcoded host paths. bash scripts/review-checks.sh --diff <(git diff origin/main...HEAD): hardcoded-paths and root-artifacts clean (PARTIAL only because there is no .py in the diff for prose-cap). A grep of the added lines for /Users/, /home/, /tmp/, /private/ finds nothing.
CI on the head (newest run per check name, repos/sonichi/sutando/commits/<sha>/check-runs): every check success (lints + typecheck, tsc + tests clean install, node + out-of-tree python tests, python standalone 1-7, shell lanes + serial tails 1-3, eslint, ruff, shellcheck, src-map, all the refuse-* gates, smoke, diff coverage); PowerShell skipped; license/cla success. No failure to compare against main.
Nits (non-blocking)
recentAtAskis read before the spoken-turn wait, so a late ASR chunk that lands during the wait is no longer in the "recent voice transcript" block; it is carried only by the verbatimspokenBlock. That is consistent with the fix's intent (the context is the one the task was asked in), but it is a small change to what the transcript block contains, and the body does not call it out. Worth one line in the body so the next reader does not take it for an accident.src/task-bridge.ts:855runsgetRecentConversation(4)on every call, including the ones that return early (thedescribe_screenredirect at 862 and the video fast paths). It is best-effort and cheap; just noting it is now unconditional.
Verified
- The three source changes and that no other
_voiceSessionOriginread remains inexecute(). - The three named test files at the head (36/36).
- The head's new test file against the main tree (2 new cases fail with
place-bvsplace-a; the 14 existing cases pass). - review-checks (hardcoded paths, root artifacts) clean; no absolute host paths in added lines.
- CI on the head: all green, CLA green.
Not verified
- A live voice session moving origin inside a call's in-flight window (the body says the same; the deterministic tests drive the real
workTool.execute, which is the best available stand-in). npm run test:tsin full; I ran the three files the body names and rely on the greentsc + tests (clean install)check for the rest.
This is a COMMENT review only: the GitHub login that posted it is shared, so formal approval is the owner's own click.
— Sutando (qingyun-001)
marklysze
left a comment
There was a problem hiding this comment.
Approve at c3961824 (full c39618245a0d86631c73d1e88ab422d327473001, base main). I did not merge. I was asked to review this and I am adding evidence the existing approvals do not have, not a repeat of them: I ran the test, reproduced the failure on the parent, and mutated nine decisions.
Verified
- The bug exists on the parent and the new tests catch it. With
origin/main'ssrc/task-bridge.tsand this PR's test file, cases 4 ("a session that moves during the spoken-turn wait leaves the task with its own origin and words") and 5 ("moves right after the call is made") fail. At this headtests/task-bridge-voice-origin.test.tsis 16 of 16 pass (I bundled it with esbuild and ran it withnode --test, since this host has notsx), and so istests/task-bridge-format.test.ts(11 of 11). - There is no
awaitbetween the start ofexecuteand the origin read (lines 844-852; the onlyawaitin that span is inside the comment), so the "read before the first await" claim holds. The only other readers of the live origin are thegetVoiceSessionOrigin()getter (voice-agent.ts:1144, inline-tools.ts:739) and the rest ofexecutenow uses the captured value. - Mutation, 9 decisions, 7 caught: the origin read after the awaits, the generation never bumped, the generation ignoring a target change, the spoken wait not bound to the session, no check on the first read, the conversation read after the wait, and the conversation snapshot dropped.
- Worst case: the change adds one best-effort
getRecentConversation(4)call at entry, including on the early-return paths (the screen redirect and the dedup return); it is wrapped intry. A moved session now gets nospokenblock rather than the new session's words. No opt-in, no state format and no path change. No hardcoded host paths in the added lines.
Not blocking: two survivors (untested claims)
- Re-check after the polled read. Replacing the loop's
read()with a bare_spokenTurns(count)passes, so the claim that a move during a read is re-checked after every read is pinned only for the first read (removing that check is caught). The deterministic test moves the session on the first read. - A channel-only change bumping the generation. Making the condition ignore the channel and compare only the target passes, so the "channel or target" half of that rule is untested.
Adjacent, not changed here: inline-tools.ts:739 reads the live origin for cancel (cancelOrigin) at call time; I did not look at whether a move can matter there.
Not verified: tests/task-bridge-spoken-block.test.ts (it imports bodhi-realtime-agent, which I could not install here), the full npm run test:ts (the PR reports 1640 tests, 1637 pass), and CI as a whole beyond reading the check list (all green or skipped at this head; the PR reads CLEAN and APPROVED).
What changed, and why
A voice task could be bound to the wrong origin when the live session moved to another origin while
workTool.execute(src/task-bridge.ts) was in flight. Three paths did this:execute(): the watcher probe's dynamic imports and the concat fast path. A move during any of them filed the task, its header and its result under the new origin.The fixes:
execute()entry, before any await. An async function runs synchronously up to its first await, so nothing can move in between.setVoiceSessionOriginbumps the generation whenever the bound origin's channel or target changes._awaitSpokenTurnstakes an optionalisCurrentpredicate. It discards anything read once the session has moved, re-checking after every read, because the move can happen during the read itself. Other callers keep the old behaviour by default.Where to look first
src/task-bridge.ts:workTool.execute;_awaitSpokenTurns;setVoiceSessionOrigin.What user behavior or bug does this prove?
A task asked in one place could be answered in another, and carry that place's words.
tests/task-bridge-voice-origin.test.tshas two deterministic cases with no timers:Feature/documentation consistency
N/A: a bug fix with no feature or capability change.
Before/after evidence
Parent
54d6465f, new tests only:This head:
Each part of the fix was reverted on its own, and each revert fails a named assertion:
Tests run
npx tsx --test --test-force-exit tests/task-bridge-voice-origin.test.ts tests/task-bridge-spoken-block.test.ts tests/task-bridge-format.test.ts: exit 0.npm run test:ts: 1640 tests, 1637 pass, 0 fail.bash scripts/review-checks.sh --diff <(git diff origin/main...HEAD): hardcoded-paths and root-artifacts clean. The result is PARTIAL only because the diff has no.pyfile for prose-cap.CI note
This head merges main after #5166 reverted #4303, which removes the agy notifier failures. If
tests/external-core-helpers.test.pyfails, it also failed on main at the docs-only commit f7b69fc (CI run 37543453454) after passing at d9ad873 and 8bf2d16. It passes 22/22 in three local runs here, and this change does not touch it.Live path
This touches the voice → task delivery path. No live round trip was captured. It needs a live session to change origin within a call's in-flight window, which can't be staged on demand. The deterministic tests drive the real
workTool.executethrough those interleavings.🤖 Generated with Claude Code