feat(runtime): implement ask_user tool for plan-mode user interaction#582
Conversation
Add backend closed-loop for ask_user: tool schema with blocking broker, plan-mode-only visibility, ActionTypeInteraction security type, and gateway protocol with user_question_answer frame validation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Replace sequential ask-N IDs with crypto/rand hex IDs in Broker - Add Broker.PendingIDs() for test-time ID discovery - Use emitRunScopedPriority for critical user_question_requested event to avoid silent drops under channel pressure - Update all tests to resolve via dynamic ID instead of hardcoded "ask-1" Addresses PR 1024XEngineer#582 review from fennoai. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Thanks for the thorough review. Here's what was addressed in da38a6f: 1. Predictable request IDs (high) — Fixed. 2. Event delivery reliability (medium) — Fixed. The critical 3. TUI event wiring — This is intentional scope boundary. Issue #578 Plan A is explicitly Tool + Runtime + Gateway only (no TUI). The event types are defined and emitted so the TUI layer can consume them in a follow-up. The runtime contract extension and TUI rendering are separate work. 4. Remote adapter wiring — Same as #3. This is the backend closed-loop. The gateway protocol ( All tests pass with no regressions. |
|
/review |
There was a problem hiding this comment.
I found a few blocking issues in the new ask_user flow. The main one is that the request event is emitted before the broker-generated request_id exists, so clients cannot send a valid gateway.user_question_answer back. I also found one security issue in request-id generation and one error-classification regression in the tool error path.
|
|
||
| // emit user_question_requested before blocking | ||
| if call.AskUserEventEmitter != nil { | ||
| call.AskUserEventEmitter("user_question_requested", request) |
There was a problem hiding this comment.
user_question_requested is emitted before broker.Open returns, so this payload never contains the broker-generated request_id. That breaks the new control-plane round trip: gateway.user_question_answer requires payload.request_id, but the client never receives one here. As written, the first ask_user call will block until timeout because the UI/gateway has no valid identifier to answer.
| // newRequestID generates an unguessable request ID using crypto/rand. | ||
| func newRequestID() string { | ||
| buf := make([]byte, 8) | ||
| _, _ = rand.Read(buf) |
There was a problem hiding this comment.
Ignoring the rand.Read error defeats the “unguessable ID” guarantee. On entropy failure buf stays zeroed, so every request becomes the same ask-0000000000000000 ID, which can overwrite an in-flight entry in pending and misroute or hijack answers. This should fail closed or retry instead of silently accepting a predictable/colliding ID.
| QuestionID: request.QuestionID, | ||
| Message: err.Error(), | ||
| }) | ||
| return ToolResult{ |
There was a problem hiding this comment.
The error branch returns err != nil with ToolResult.IsError still false. The runtime persists tool messages based on result.IsError, so an ask_user timeout/cancellation will be recorded as a successful tool result instead of a failed one. That regresses the existing tool-error invariant and can mislead both the model and the UI about what happened.
|
/code CI炸了，找下原因修复一下，修好推送到我的分支 |
Result已定位并修复两个 CI 问题，变更已推到
Verification本地已通过 |
Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: Cai-Tang-www <106404101+Cai-Tang-www@users.noreply.github.com>
|
/code 提高测试覆盖率，然后推送到我的分支 |
Result已提高这次
Verification已通过 Push提交： |
Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: Cai-Tang-www <106404101+Cai-Tang-www@users.noreply.github.com>
|
/code CI炸了，找下原因修复一下，修好推送到我的分支 |
Result已修复这次 CI 失败，并推送到当前 PR 分支。 这次炸掉的不是 修复内容：
Verification已通过：
Push提交： |
Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: Cai-Tang-www <106404101+Cai-Tang-www@users.noreply.github.com>
test(runner): tolerate handshake transport errors
3 participants
Summary
ask_usertool with JSON schema, blocking broker, and plan-mode-only visibilityActionTypeInteractionsecurity type for user interaction toolsuser_question_answerframe validationRelated Issue
Closes #578
Test plan
TestBuildPermissionActionandTestValidateFrametests pass🤖 Generated with Claude Code