Repository navigation
Conversation
|
@yixuan-ag2 Chi asked for your review on this one. Summary: the voice 🤖 Generated with Claude Code |
…a docked room's bound worker by default The voice `work` tool takes an optional `worker` name. The worker-pool skill's voice hook resolves it against the roster (case and spacing ignored) and the task carries the id as `requested_worker`; an unknown or ambiguous name is told to the user and nothing is written. The core reaches the roster only through an injected resolver (SkillSetupCtx.setVoiceWorkerResolver), so it names no skill. A work task docked in a room already carries the room as `channel_id`, so the room's bound worker gets it; the session-end task now does the same, with the routing header only (no adapter keys), latched across the disconnect that releases the binding before the hang-up grace runs out. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a415c43 to
857e704
Compare
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 94%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
liususan091219
left a comment
There was a problem hiding this comment.
Requesting changes for one blocker: the PR's own CI is red, and the red is caused by this diff.
Blocker: the tool-table anchor fixture was not regenerated
- CI job "node + out-of-tree python tests" (run 38312145854, job 115020232128) has 1 failure out of 1703:
not ok 450 - tool-table anchor matches the committed fixtureintests/voice-behavior-anchors.test.ts. - The diff shown is the new
workerparameter onworkTool(src/task-bridge.ts, theworker: z.string().optional().describe(...)hunk). The fixturetests/fixtures/voice-behavior-anchors.jsondoes not have it. - "tsc + tests (clean install)" fails only because it gathers that job's result (
node-and-oot-tests FAILED). - The last three CI runs on
mainare green (head 3208059 and the two before it), so this is not inherited from main. - The anchor is there because tool descriptions are the tuned prompt surface. So this needs a deliberate regeneration, not a quiet fix.
Asks:
- Regenerate on purpose (
ANCHOR_UPDATE=1 npx tsx tests/voice-behavior-anchors.test.ts) and commit the fixture change. - Say in the PR body that the
worktool's parameter table, which voice reads, gainsworker. Quote the new description string. The body's "Tests run" list leaves this suite out, so it was not run before push. - Re-run CI and get it green.
Not blocking, named: the live voice round trip has not been done, and the body says so. That covers the docked "ask Pro-dev to …" landing in deliveries/<id>/, and a docked hang-up whose session-end task routes to the room's worker and replies in the DM. Please paste it when you have it.
What I checked and found fine
- New tests exercise real behavior. They call the real
workTool.execute, the realresolve_worker_name.pyagainst a fixture roster, andpool_router.route. They do not grep source. The changedvoice-session-end.test.tssource regexes are an existing pattern, updated in place, not new. resolve_spoken_namesits next toresolve_labelinpool_roster.py, so the policy has one owner. Core reaches it only through the injectedsetVoiceWorkerResolverseam.requested_workeris checked againstREQUESTED_WORKER_REbefore it is written. A named worker with no resolver installed is refused, not sent to the core.- Hardcoded-path scan over the added lines found no
~/.sutando,$HOME/.sutandoor/Users/<name>literals.SUTANDO_WORKSPACEappears only as a test-scoped env set intests/voice-worker-routing.test.ts, not as a live default. - license/cla shows success, with 1 context at 857e704.
Not verified: I did not run any test locally. Everything above comes from the diff and the CI logs. I did not check whether #5429 or #5035 conflict at this head.
— Sutando (susans-MacBook-Pro)
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@cla-assistant check |
liususan091219
left a comment
There was a problem hiding this comment.
Changes still requested at 921e903c. The only commit since my review at 857e704b is 921e903c, a merge of main. It does not address the blocker, and CI is red at this head for the same reason.
Blocker (unchanged): the tool-table anchor fixture was not regenerated
- CI run 40232216012, job "node + out-of-tree python tests": 1961 pass, 1 fail,
not ok 506 - tool-table anchor matches the committed fixture(tests/voice-behavior-anchors.test.ts). - The diff in the failure is exactly this PR's new param:
worker: 'Only when the user names a worker to do it ("ask Pro-dev to …"): …'on theworktool (src/task-bridge.ts, theworker: z.string().optional()hunk inworkTool.parameters).tests/fixtures/voice-behavior-anchors.jsonstill lacks it. - "tsc + tests (clean install)" fails only because it gathers that job's result.
Asks (same as before):
- Regenerate on purpose (
ANCHOR_UPDATE=1 npx tsx tests/voice-behavior-anchors.test.ts) and commit the fixture. - Say in the PR body that the
worktool's parameter table gainsworker, and quote the description string. - Get CI green.
The merge itself checks out. git show --remerge-diff shows conflicts in src/task-bridge.ts, src/voice-agent.ts, src/skill-setup-runner.ts and docs/catalog.json, and every one keeps both sides:
buildVoiceTaskHeadertakes main'slaneand then this PR'srequestedWorker. It writes both header lines. The caller andtests/voice-worker-routing.test.tswere updated to the new argument order.- The worker lookup in
workTool.executenow runs aftercaptureWorkOrigin(). That matches main's "read before the first await" rule, sinceresolveVoiceWorkeris awaited. SkillSetupCtxandvoice-agent.tskeep bothsetVoiceWorkerResolverand main'sonUserSpoke.catalog.jsontakes main's newerlast_verified.
Gates: license/cla success (1 context). No unresolved threads.
Not blocking, named: the live voice round trip is still owed (the docked "ask Pro-dev to …" landing in deliveries/<id>/, and a docked hang-up whose session-end task routes to the room's worker).
Not verified: I read the merge resolution and the CI log; I did not run the node suite locally.
— Sutando (susans-MacBook-Pro)
What changed, and why
Chi approved letting voice tasks go to workers (AG2 Space DM, 2026-10-12 21:25Z).
worktool takes an optionalworkerparameter. "Ask Pro-dev to …" passes the name as said. The worker-pool skill resolves it against the roster, ignoring case and spacing (Pro-dev,pro devandPRO_DEVall matchpro-dev), and the task carries the worker id asrequested_worker. If the name matches no worker, or more than one, voice tells the user and lists the workers, and no task file is written. Before this PR the parameter didn't exist, so the name was dropped and the task went to the core.channel_id, so the router gives it to the room's bound worker. With no room it goes to the core. This PR pins that with tests and makes an explicit name override it. The[dm-only]handling is unchanged, because the origin and its headers are unchanged.channel_id, and nothing else. The adapter keys (source_room_id,channel_kind) and theroom_contextline are left out, so no room origin is rebuilt for its result. The result still goes the owner-DM way that feat(voice): the session-end task files voice-agent problems as pending questions; its result reaches the owner's DM, not a later call #5429 sets up. The prompt text is not touched.The client disconnect releases the binding about 60s before the hang-up grace runs out. So
createCallOriginLatchrecords the origin at disconnect and clears it on reconnect. A call still running at shutdown uses the live origin.How roster labels are reached without core importing the skill
src/task-bridge.ts) only has an injected seam:setVoiceWorkerResolver(fn), added toSkillSetupCtxthe same waysetVoiceTaskOriginResolverwas. With no resolver installed, a named worker is refused ("No workers are set up here…"). It is never sent to the core. The core contains no roster code and no worker-pool path;tests/core-never-names-the-worker-pool-skill.test.pystill passes.skills/worker-pool/manifest.jsonandtools.tsare a thinsetup(ctx)hook with no tools. Its resolver runsscripts/resolve_worker_name.py, which calls the newpool_roster.resolve_spoken_name. That function sits next toresolve_label, so the matching rules live in one place, in the pool. The script resolves the workspace the same way the router does.requested_workeris the worker id. The router accepts an id as-is, so a later display-label change can't move the task to someone else.Review first
src/task-bridge.ts:resolveVoiceWorker,buildVoiceTaskHeader,_routingOnlyOriginandsubmitVoiceSessionEndTask,createCallOriginLatch, and theworkerparameter inworkTool. Thenskills/worker-pool/scripts/pool_roster.py(resolve_spoken_name) andskills/worker-pool/tools.ts.Feature/documentation consistency
docs/worker-pool-design.md→ Routing gets a "Voice" paragraph, and itslast_verifiedindocs/catalog.jsonis updated.skills/MANIFEST.mdlists the new ctx member.Before / after evidence
Demo script (
demo-voice-route.mts, not committed). It calls the realworkTool.executeandsubmitVoiceSessionEndTask, then sends each task file through the realpool_route_handler.read_taskandpool_router.route. The fixture roster haspro-dev=w-dev,pro-comm=w-comm,pro-fast=w-fast, and the room is bound tow-comm.Parent
4e22807db(origin/main):HEAD:
New tests, run at the parent (the test files copied into an
origin/mainworktree):At HEAD:
Tests run
npx tsx --test --test-force-exit tests/voice-worker-routing.test.ts tests/voice-session-end.test.ts tests/ag2space-voice-room.test.ts tests/skill-setup-dedup.test.ts tests/skill-setup-isolation.test.ts tests/voice-plugin-seam.test.ts tests/task-bridge-*.test.ts tests/relay-agent*.test.ts: 293 pass, 0 failpython3ontests/requested-worker-header.test.py,core-never-names-the-worker-pool-skill.test.py,lint-skill-supervised-worker.test.py,worker-pool-design-retractions.test.py,marketplace.test.py,health-check-dangling-symlinks.test.py,pool-router-replay-idempotence.test.pyandpool-router-non-exclusive-folder.test.py: all PASS.bash tests/start-cli-skill-manifest-config.test.sh: PASS.npx tsc --noEmitandnpx tsc -p tsconfig.skills.json: no errors in the changed files.eslinton the changed files: 0 errors; the warnings are existingno-explicit-anyones intask-bridge.ts.git diff --cached | bash scripts/review-checks.sh: PASS.tests/voice-session-end.test.ts: the source-shape assertions for thevoice-agent.tswiring were updated to the new lines (callOrigin.clientConnected()/clientLeaving()/take()).Live path
This touches the voice task writer. No live voice round trip has been done yet. Evidence so far is the real tool, task-file and router path against a fixture roster. Before merge, a live witness should do two things on a host with the pool:
deliveries/<pro-dev id>/gets the task;Coordination
submitVoiceSessionEndTask's task-id prefix and the session-end prompt. This PR changes only its signature and routing header, plus three lines oftests/voice-session-end.test.ts. Expect a small textual conflict; whichever lands second rebases.workTool.executeentry) touches the same function. The worker name is resolved after the origin and recent turns are read, so it keeps the "read before the first await" order.🤖 Generated with Claude Code