Skip to content

fix(codex): isolate subagent app-server events - #1656

Open
AaronZ345 wants to merge 1 commit into
chenhg5:mainfrom
AaronZ345:agent/codex-filter-subagent-events-20260809
Open

fix(codex): isolate subagent app-server events#1656
AaronZ345 wants to merge 1 commit into
chenhg5:mainfrom
AaronZ345:agent/codex-filter-subagent-events-20260809

Conversation

@AaronZ345

Copy link
Copy Markdown
Contributor

Supersedes #1520. This is the same patch on a fresh branch after the 30-day maintenance gate; the prior PR had no requested changes or unresolved review threads.

Summary

  • ignore Codex app-server lifecycle notifications that belong to subagent threads instead of the active cc-connect thread
  • scope item, completion, and token-usage notifications to the active parent turn
  • preserve compatibility with older app-server notifications that omit thread or turn IDs
  • add regression coverage proving child turns cannot overwrite or complete the parent turn

Root cause

Codex app-server multiplexes parent and subagent events over the same connection. Its v2 protocol includes threadId on turn/started, turn/completed, and thread/status/changed, plus both threadId and turnId on item and token-usage notifications.

cc-connect parsed those fields but did not validate them. When a subagent started, its turn/started notification replaced the parent's currentTurn and cleared pending parent messages. When that child thread became idle or completed, cc-connect called completeTurn() and emitted an empty EventResult. Repeated child turns therefore produced repeated empty responses, while parent output could be truncated. Child token usage could also overwrite the parent turn's usage snapshot.

This change fixes the event-routing boundary rather than suppressing empty messages. Parent-thread follow-up turns, including results delivered back from subagents, continue through the normal unsolicited-event path.

Live evidence

In a GPT-5.6 Sol ultra run, the parent turn completed at 12:15:08. Codex then started subagent turns on distinct thread IDs. Child thread idle transitions at 12:15:15 and 12:15:23 aligned exactly with cc-connect logging unsolicited turn complete response_len=0. The Codex 0.144.1 generated protocol schema confirms these notifications carry the thread and turn identifiers needed for routing.

Validation

  • go test ./agent/codex -count=1
  • go test -race ./agent/codex -run 'TestAppServerSession_(IgnoresSubagentLifecycleNotifications|IgnoresSubagentTokenUsageNotifications)' -count=1
  • go test -tags no_web ./cmd/cc-connect ./agent/codex -count=1
  • go vet ./agent/codex
  • git diff --check

@chenhg5 chenhg5 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

结论: Approve

总体判断: 一个 critical Codex event-routing fix——subagent lifecycle notifications 之前会覆盖 parent turn state,导致「subagent 触发 idle → cc-connect 错误调用 completeTurn(parent) → emit empty EventResult」的严重 bug。修复后按 threadId 过滤,只处理 active parent thread 的事件。这是 routing 层的修复,不是「suppress empty messages」的 workaround,保留了 unsolicited-event 路径对 parent follow-up 的正确处理。建议合入。

Review 范围:

  • 看了 agent/codex/appserver_session.go 中 subagent 事件过滤的改动(+35/-6)。
  • 看了新增 2 个 -race 测试:IgnoresSubagentLifecycleNotifications + IgnoresSubagentTokenUsageNotifications
  • 看了 PR body 中的 live evidence(GPT-5.6 Sol ultra run,subagent thread 触发 parent unsolicited turn complete response_len=0)。
  • CI: 2 次 run 全绿(首次 + rebase 后)。

✅ 做得好的地方:

  • 修复了真正的 root cause:作者明确说「This change fixes the event-routing boundary rather than suppressing empty messages」——这是「理解协议 + 在 routing 层修」而不是「在 output 层 workaround」的成熟判断。后者会掩盖 bug 让 future reader 困惑。
  • Live evidence 精准:12:15:08 parent turn 完成 → 12:15:15 / 12:15:23 child thread idle 触发 unsolicited turn complete response_len=0。这种「时间戳对齐」的 evidence 比「我遇到过」可信度高一个数量级,让 reviewer 不必自己跑复现。
  • 保持向后兼容:「preserve compatibility with older app-server notifications that omit thread or turn IDs」——这是 protocol-level 的兼容性承诺。Codex 升级期间 cc-connect 不会因为某个 notification 字段缺失就崩溃。
  • Token usage 也过滤IgnoresSubagentTokenUsageNotifications 测试覆盖了「child token usage 不覆盖 parent usage snapshot」这条独立路径——这避免了 quota 显示被 subagent 干扰的二级 bug。
  • -race 测试覆盖两个独立场景:lifecycle(turn/started + turn/completed)和 token usage 都独立测,确保 routing fix 不只修了一条路。

🟠 建议改进(不阻塞):

  • 新增测试的命名一致性TestAppServerSession_IgnoresSubagentLifecycleNotifications 隐含了「测试一个 thread」,但 live evidence 显示 Codex 同时跑多个 subagent thread。建议加一个测试 TestAppServerSession_IgnoresMultipleConcurrentSubagents 覆盖「parent turn + 3 child subagent threads 同时 active」的场景。
  • response_len=0 现在还会出现吗? 修复后 subagent idle 不会触发 completeTurn(parent),但 parent turn 自身完成时仍会 emit EventResult。建议在测试里明确「修复后 parent turn 正常完成时 emit 一次 EventResult,且 length > 0」的 happy path。
  • thread/status/changed 单独 filter:PR body 提到「Codex v2 protocol includes threadId on thread/status/changed」,但 diff 没显示这条独立过滤。建议在 diff 里补充确认这条也按 threadId 过滤。
  • PR body 引用「The Codex 0.144.1 generated protocol schema confirms」:建议作者在 commit message 或 doc 里加一条 link 到 generated schema,方便 reviewer 验证 routing 规则。

🔵 可选优化:

  • 可以加一个 integration test(用 mock app-server 模拟「parent + 3 child 通知交错到达」),但 unit test 已覆盖核心 routing 逻辑。低优先

Testing / Risk:

  • 已看到的验证: CI 2 次全绿;-race 测试通过;2 个新增 race regression test。
  • 未覆盖风险: 多 subagent thread 并发 + parent/child notification 时序交错的真实 wire log——作者提供了 live evidence 但没贴 raw wire log。建议 post-merge 用真实 Codex CLI 0.144+ 跑 5 次 GPT-5.6 ultra,确认不再出现 response_len=0 日志。
  • Blast radius: 仅 agent/codex/appserver_session.go 的事件处理路径;不影响其他 agent。

Next step:

  • 建议 owner 直接 merge。这是一个 critical routing fix,修复「subagent 误触发 parent turn complete」的用户可见 bug,scope 小、行为正确。可以现在合。
  • post-merge 强烈建议:用真实 GPT-5.6 ultra + 至少 3 个 subagent run 跑 30 分钟,确认日志里再无 response_len=0

@AaronZ345
AaronZ345 force-pushed the agent/codex-filter-subagent-events-20260809 branch 2 times, most recently from 9804372 to 969483f Compare August 14, 2026 14:43
@AaronZ345
AaronZ345 force-pushed the agent/codex-filter-subagent-events-20260809 branch from 969483f to 8c65316 Compare August 15, 2026 14:44
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.

2 participants