Skip to content

fix: dispatch local browser sessions concurrently - #27

Merged
lanyue-llk merged 4 commits into
lex-mainfrom
codex/browser-local-concurrent-start
Aug 28, 2026
Merged

fix: dispatch local browser sessions concurrently#27
lanyue-llk merged 4 commits into
lex-mainfrom
codex/browser-local-concurrent-start

Conversation

@lanyue-llk

@lanyue-llk lanyue-llk commented Aug 28, 2026

Copy link
Copy Markdown

Summary

  • start browser media capture and explicit agent dispatch concurrently for local browser sessions
  • preserve the existing concurrent startup order for sandbox server-managed input
  • add regression coverage for browser startup without a sandbox id and sandbox server-managed startup

Root cause

Local browser sessions waited for media startup before dispatching the agent. The browser audio gate needs the agent controller to join, so the two sides could wait on each other and leave the UI at Starting.

Validation

  • pnpm test: 259 passed
  • pnpm lint: passed
  • pnpm format:check: passed
  • pnpm build: passed
  • local browser smoke: agent dispatch completed, face recognition and proactive greeting reached the interactive state

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown

Claude finished @lanyue-llk's task in 1m 52s —— View job


Review 结论

通过

问题及建议

  • [P2] hooks/useRoom.ts:255-256:新逻辑 usesConcurrentBrowserStartup = browserSourceClient.enabled 去掉了旧版对 appConfig.sandboxId 的判断(旧版 usesSandboxConcurrentStartup = Boolean(sandboxId) && usesManagedRoomInput)。这意味着「有 sandboxId 且仅 usesServerRoomInput(非浏览器)」的会话,之前走并发路径,现在会退化为顺序路径(先 startLocalInput() 关闭麦克风,再 dispatchAgentSession())。由于该分支只是同步禁用麦克风,不涉及等待 agent controller join,功能上应无风险,但与 PR 描述「keep server-managed input startup unchanged」不完全一致,建议确认这是有意为之。
  • [P2] tests/session-start-dispatch.test.mjs:220-232:新增测试仍沿用仓库现有风格,通过正则匹配 useRoom.ts 源码文本,未实际执行/mock Promise.allSettled 并发路径与错误处理分支(如 localInputResultdispatchResult 同时 rejected 的场景)。与文件内既有测试风格一致,非本 PR 引入的新问题,可后续视需要补充运行时用例。

核心修复逻辑正确:将 usesConcurrentBrowserStartup 直接绑定到 browserSourceClient.enabled(不再依赖 sandboxId),解决了本地浏览器会话中 browserSourceClient.start() 内部 audioGate.adapter.start()hooks/useBrowserSourceClient.ts:511)等待 agent controller 加入房间、而 agent dispatch 却排在其后才触发导致的相互等待/卡在 "Starting" 问题,改动范围小且与描述的根因吻合,未见无关改动。

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown

Claude finished @lanyue-llk's task in 1m 28s —— View job


任务清单

  • 获取 PR diff 与相关代码上下文
  • 审查 hooks/useRoom.ts 改动
  • 审查 tests/session-start-dispatch.test.mjs 改动
  • 输出审查结论

Review 结论

不通过

问题及建议

  • [P0] hooks/useRoom.ts:256tests/browser-room-session.test.mjs:147-150:变量 usesSandboxConcurrentStartup 已被重命名为 usesConcurrentBrowserStartup,但既有测试 tests/browser-room-session.test.mjs 仍以正则断言源码包含字符串 usesSandboxConcurrentStartup = Boolean(appConfig.sandboxId) && usesManagedRoomInput。该字符串在改动后已不存在,pnpm test(脚本为 tsx --test tests/*.test.mjs,会跑到这个文件)将会失败。PR 描述里的验证只跑了 node --test tests/session-start-dispatch.test.mjs,未跑全量测试套件,漏掉了这个回归。建议同步更新/删除 tests/browser-room-session.test.mjs:136-169 中对旧变量名、旧并发条件的断言(该用例标题为 "sandbox browser starts media and dispatch concurrently...",语义上也需要随之调整)。
  • [P2] hooks/useRoom.ts:255-256usesConcurrentBrowserStartup 直接绑定到 browserSourceClient.enabled,去掉了旧版 Boolean(appConfig.sandboxId) 的判断。这意味着「有 sandboxId 且仅 usesServerRoomInput(非浏览器)」的会话,之前走并发路径,现在退化为顺序路径(先 startLocalInput() 关闭麦克风,再 dispatchAgentSession())。该分支只是同步禁用麦克风,不涉及等待 agent controller join,功能风险低,但与 PR 描述「keep server-managed input startup unchanged」不完全一致,建议确认是否有意为之并在描述中说明。
  • [P2] tests/session-start-dispatch.test.mjs:220-232:新增测试仍是对 useRoom.ts 源码文本做正则匹配,未真正执行 Promise.allSettled 并发路径及其错误分支(如 localInputResultdispatchResult 同时 rejected 的场景)。与文件现有风格一致,非本 PR 引入的新问题,可后续视需要补充运行时用例。

核心修复思路正确:将本地浏览器会话的并发判断从依赖 sandboxId 改为直接依赖 browserSourceClient.enabled,解决了 browserSourceClient.start() 内部音频门等待 agent controller 加入房间、而 dispatch 排在其后才触发导致的相互等待问题。但 P0 项会导致 CI 测试失败,需先修复。
· branch codex/browser-local-concurrent-start

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown

Claude finished @lanyue-llk's task in 1m 2s —— View job


任务清单

  • 获取 PR diff 与相关代码上下文
  • 审查 hooks/useRoom.ts 改动
  • 审查 tests/browser-room-session.test.mjstests/session-start-dispatch.test.mjs 改动
  • 输出审查结论

Review 结论

通过

问题及建议

  • [P2] hooks/useRoom.ts:255-256usesConcurrentBrowserStartup 已改为仅绑定 browserSourceClient.enabled,不再包含旧版 Boolean(appConfig.sandboxId) 条件。这意味着「有 sandboxId 且仅 usesServerRoomInput(非浏览器)」的会话,会从并发路径退化为顺序路径(先 startLocalInput() 同步禁用麦克风,再 dispatchAgentSession())。该分支不涉及等待 agent controller join,功能风险低,但与 PR 描述「keep server-managed input startup unchanged」不完全一致,建议确认是否有意为之,或在描述中补充说明。
  • [P2] tests/session-start-dispatch.test.mjs:220-229tests/browser-room-session.test.mjs:136-166:两处测试仍通过正则匹配 useRoom.ts 源码文本,未真正执行 Promise.allSettled 并发路径及 localInputResult/dispatchResult 同时 rejected 的错误处理分支。与仓库现有测试风格一致,非本次改动引入的新问题,可后续视需要补充运行时用例。

之前一轮审查发现的 P0 问题(tests/browser-room-session.test.mjs 仍断言已重命名的 usesSandboxConcurrentStartup 导致 pnpm test 失败)已在最新提交 62ab3a6 中修复:该测试已更名为 "browser input starts media and dispatch concurrently",并改为断言新变量名 usesConcurrentBrowserStartup = browserSourceClient.enabled,与 tests/session-start-dispatch.test.mjs 中的新增测试保持一致。核心修复逻辑(将本地浏览器会话的并发判断从依赖 sandboxId 改为直接依赖 browserSourceClient.enabled)正确,解决了 browserSourceClient.start() 内部音频门等待 agent controller 加入房间、而 dispatch 排在其后触发导致的相互等待问题,改动范围小且未见无关改动。

注:本环境未获得执行 pnpm test / npx tsx --test 的 Bash 权限,未能在本地重新跑通全量测试套件验证修复,以上结论基于静态审查源码得出。

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown

Claude finished @lanyue-llk's task in 59s —— View job


任务清单

  • 获取 PR diff 与相关代码上下文
  • 审查 hooks/useRoom.ts 改动
  • 审查 tests/browser-room-session.test.mjstests/session-start-dispatch.test.mjs 改动
  • 输出审查结论

Review 结论

通过

问题及建议

  • [P2] tests/session-start-dispatch.test.mjs:220-241tests/browser-room-session.test.mjs:136-169:新增/修改测试仍通过正则匹配 useRoom.ts 源码文本,未实际运行 Promise.allSettled 并发路径及 localInputResult/dispatchResult 同时 rejected 的错误处理分支。与仓库现有测试风格一致,非阻塞项,可后续补充运行时用例。

未发现其他需要阻塞合并的问题。残余风险:本环境无 Bash 执行权限,未能本地重跑 pnpm test / npx tsx --test tests/*.test.mjs 验证,以上结论基于静态审查源码得出。

核心修复逻辑正确:最新提交 064f18dusesConcurrentManagedStartup 调整为 browserSourceClient.enabled || (Boolean(appConfig.sandboxId) && appConfig.usesServerRoomInput)hooks/useRoom.ts:256-258),既解决了本地浏览器会话(无 sandboxId)中 browserSourceClient.start() 内部音频门等待 agent controller 加入房间、而 dispatch 排在其后触发导致的相互等待问题,又保留了「sandboxId + usesServerRoomInput」场景下原有的并发启动路径,与 PR 描述「keep server-managed input startup unchanged」一致,也修复了此前几轮审查中指出的行为回退问题。改动范围小,变量命名(usesSandboxConcurrentStartupusesConcurrentManagedStartup)在源码与测试中均已同步更新,未见无关改动。
· branch codex/browser-local-concurrent-start

@lanyue-llk

Copy link
Copy Markdown
Author

Review follow-up

已清点本 PR 的全部评论:PR body 1 条、top-level comments 3 条,formal reviews、inline comments 和 unresolved threads 均为 0。

处理结果:

  • P0 已在 62ab3a6 修复:全量测试不再引用旧变量名。
  • 重复出现的 P2 已在 064f18d 修复:browser 输入在 sandbox 内外都并发启动;sandbox 的 server-managed input 恢复原有并发启动顺序,PR 描述与实际行为一致。
  • 关于把源码契约测试改成完整 hook 运行时测试的建议本次不展开。现有仓库采用源码契约测试覆盖该编排;本次补充了 browser 和 sandbox server-managed 两条行为条件。为测试而拆分整个 hook 会扩大修复范围。

验证结果:

  • pnpm test: 259 passed
  • pnpm lint: passed
  • pnpm format:check: passed
  • pnpm build: passed

@lanyue-llk
lanyue-llk merged commit 61cc513 into lex-main Aug 28, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant