feat(codex): steer busy follow-ups into the active turn - #1652
Open
ternary-chen wants to merge 2 commits into
Open
feat(codex): steer busy follow-ups into the active turn#1652ternary-chen wants to merge 2 commits into
ternary-chen wants to merge 2 commits into
Conversation
This was referenced Aug 7, 2026
ternary-chen
marked this pull request as ready for review
August 8, 2026 09:53
chenhg5
approved these changes
Aug 13, 2026
chenhg5
left a comment
Owner
There was a problem hiding this comment.
结论: Approve
总体判断: 一个大但架构清晰的 Codex feature——把「busy follow-up 排队等待 turn 完成」升级为可选「steer 注入到 active turn」。新引入 optional SteerTurn interface method(不动既有 Agent),config 默认 queue(opt-in steer),steering 失败 fallback 到 queue,attachments 保持 queue 语义。修复「/ps 误开 concurrent second turn」的潜在 race + 提供 user-visible「busy 时改主意立刻生效」能力。建议合入。
Review 范围:
- 看了
core/interfaces.go中新增SteerTurninterface method(+8 行)。 - 看了
agent/codex/appserver_session.go中 SteerTurn 实现(+53/-0)。 - 看了
core/engine.go中 busy follow-up routing(+99/-3)—— steer vs queue 决策 + watermark advance。 - 看了
config/config.go+config.example.toml中busy_message_modeconfig + 文档同步。 - 看了 11 个新测试:steer RPC、reject stale、config parsing、busy steer、queue default、steer failure fallback、attachment queued、/ps safety、CUJ A8。
- CI: run 31178052918 全绿(lint / unit / smoke / regression / performance)。
✅ 做得好的地方:
- Optional interface method:新加
SteerTurn而非修改既有Send,所以未实现 SteerTurn 的 agent(claudecode / pi / cursor / qoder / ...)完全不受影响。这是最小侵入式扩展核心 interface 的标准做法。 - Config opt-in 默认 queue:
busy_message_mode = "steer"必须显式配置才启用,默认行为不变。这是「feature adoption 不破坏既有 user」的明智选择。 - Steering 失败 fallback queue:
TestHandleMessage_SteerFailureFallsBackToQueue覆盖「steer RPC 失败 → 消息进入 queue」——这是「capability 不可用时优雅降级」的标准 pattern。 - Attachments 保持 queue 语义:
TestHandleMessage_BusyAttachmentRemainsQueued显式断言「带图片/文件的消息即使 steer 模式也走 queue」——这是「steer 的 wire 协议只支持 text,attachments 必须 fallback」的工程妥协,避免了「steer 时 attachments 静默丢失」的回归。 /ps不再开 concurrent turn:TestCmdPs_BusySessionWithoutSteeringCapabilityFailsSafely是 critical regression test —— 之前/ps在 busy session 调Send可能开第二个 turn(race),修复后/ps必须 SteerTurn 接口,未实现接口直接 fail-safe。这与 PR #1665 的 fix 同源,但 #1665 是协议层 fix(用turn/steer),#1652 是 interface 层 fix(要求 capability)。- CUJ A8 端到端测试:
TestCUJ_A8_BusyFollowUpSteersCurrentTurn覆盖「task → busy follow-up → history」完整 user journey,把 unit test 整合成「真实使用」的 contract。 - 本地 Feishu 验证:作者明确说「multiple busy follow-ups were incorporated into one active Codex turn and that the resident app-server process was reused by a later turn」——真实平台验证 + app-server process 复用观察,比单纯 unit test 更可信。
🟠 建议改进(不阻塞):
SteerTurninterface 名字 vsAgentSession.Send:现有Send是「发送并开 turn」,但SteerTurn是「追加到 active turn」。建议在 interface 注释里明确「SteerTurn must be called only when an active turn exists; Send starts a new turn」,避免误用。busy_message_modeconfig 的 i18n / 用户文档:当前只在config.example.toml加了 doc,但 cc-connect docs 目录下没有更新。建议作者 follow-up 加docs/configuration.md条目。TestHandleMessage_SteerFailureFallsBackToQueue测的失败类型:当前应该 mock 一个返回 error 的 RPC。建议明确覆盖「网络错误」「turn/steer 协议错误」「turnID mismatch」三种失败路径。- CUJ A8 测试用真实 Feishu 验证:作者在 PR body 里写了真实验证,但测试代码应该也加一个 integration test(用 mock platform)覆盖 busy follow-up → ack message → history integration。低优先。
- PR 体积 714 行仍较大:建议 owner merge 后在 release notes 显式列出「新增 busy_message_mode = "steer」——这是 user-visible feature,需要在 v1.4.x release notes 显式列出。
🔵 可选优化:
busy_message_mode当前只支持queue和steer两个值,建议未来加priority(steer text + queue attachment)作为第三选项,但当前 scope 已足够。保留。
Testing / Risk:
- 已看到的验证: 11 个新测试覆盖 6 个独立维度(RPC / config / engine / /ps / attachments / CUJ);CI 全绿;-race 通过;本地 Feishu 验证。
- 未覆盖风险: 真实 GPT-5.6 ultra + busy follow-up 5+ 次的场景下 steer 行为(PR body 没贴 raw wire log,但本地验证已说明)。
- Blast radius: 仅
core/interfaces.go加 optional method + codex adapter 实现 + engine 路由逻辑;不影响未实现 SteerTurn 的 agent。 - Windows baseline failure 是 pre-existing(与本 PR 无关)——作者明确指出。
Next step:
- 建议 owner 直接 merge。这是一个 well-architected feature,scope 大但 interface opt-in 设计最小侵入,steering failure fallback 安全,attachments 语义保留,CUJ 端到端测试覆盖。可以现在合。
- post-merge 验证: 用真实飞书 + GPT-5.6 ultra 跑「启动 task → 立即发 3+ busy follow-up → 确认 ack → 确认 history 显示所有 follow-up 已 inject」端到端。
- release notes: 显式列出「新增 busy_message_mode = "steer" opt-in config」。
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add an optional agent-session steering capability and implement it for the Codex app-server backend.
Projects may opt into
busy_message_mode = "steer"so plain-text follow-ups received during an active turn are appended to that turn instead of waiting in the FIFO queue. Queue mode remains the default for backward compatibility.Steering failures and unsupported sessions fall back to the existing queue. Messages containing images or files also remain queued.
/psnow requires the steering capability and no longer risks starting a concurrent second turn viaAgentSession.Send.Type of change
Testing
Automated tests added in this PR
TestAppServerSession_SteerTurnUsesExpectedActiveTurninagent/codex/appserver_session_test.go— sendsturn/steerwith the active thread and expected turn ID.TestAppServerSession_SteerTurnRejectsMissingActiveTurn— rejects stale or idle steering without writing an RPC request.TestLoad_ParsesBusyMessageModeandTestLoad_RejectsInvalidBusyMessageModeinconfig/config_test.go.TestHandleMessage_BusySteerableSessionSteersPlainTextincore/engine_test.go— steers, persists history, advances the watermark, and acknowledges the user.TestHandleMessage_BusySteerableSessionDefaultsToQueue— preserves queue as the default.TestHandleMessage_SteerFailureFallsBackToQueue— does not lose a follow-up when steering fails.TestHandleMessage_BusyAttachmentRemainsQueued— keeps attachment delivery semantics unchanged.TestCmdPs_BusySessionWithoutSteeringCapabilityFailsSafely— prevents/psfrom starting a concurrent turn throughSend.TestCmdPs_BusySession_InjectsToAgentto assertSteerTurn.TestCUJ_A8_BusyFollowUpSteersCurrentTurnincore/cuj_test.go— covers the user journey from initial task through busy follow-up and history.Targeted unit tests, the complete core CUJ suite, and targeted race tests pass locally:
On Windows, repository-wide
go build ./...andgo test -race ./...are currently blocked by pre-existing cross-platform baseline failures, including an unusedosimport inagent/pi/proc_windows.go, tests requiring Unixsh/true, POSIX path and permission assertions, and optional external agent CLIs. The affected files are unchanged by this PR. The completeagent/codexpackage passes under-race.For bug fixes only — regression test
TestCmdPs_BusySessionWithoutSteeringCapabilityFailsSafelyCritical User Journeys (CUJ) impact
No CUJ touched (small refactor, doc change, etc.)
A — basic conversation
B — session lifecycle
C — agent execution control
D — security & permissions
E — scheduled tasks
F — config switching
G — error handling & robustness
H — multi-platform / multi-project isolation
I — UI rendering correctness
go test ./core/ -run TestCUJpasses locally.A new CUJ covers the user-visible busy-follow-up flow.
Manual / user-visible behavior change
With
busy_message_mode = "steer"and a steering-capable agent session:Queue mode remains the default. Local Feishu verification confirmed that multiple busy follow-ups were incorporated into one active Codex turn and that the resident app-server process was reused by a later turn.
Checklist (reviewer will verify)
go build ./...passes — blocked locally on Windows by the unchangedagent/pi/proc_windows.gobaseline issue described above.go test ./...passes — repository-wide Windows baseline failures described above; targeted and CUJ suites pass.core/.Related