-
Notifications
You must be signed in to change notification settings - Fork 7.4k
Feat: request user input tool #9472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| apply_patch_tool_type, | ||
| web_search_mode: *web_search_mode, | ||
| collab_tools: include_collab_tools, | ||
| collaboration_modes_tools: include_collaboration_modes_tools, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would only make the tool available after collaboration mode is enabled.
d16baca to
c1bcdcf
Compare
c1bcdcf to
d74863c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1bcdcf854
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub struct ToolRequestUserInputQuestion { | ||
| pub id: String, | ||
| pub header: String, | ||
| pub question: String, | ||
| pub options: Option<Vec<ToolRequestUserInputOption>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Omit options when absent to avoid null payloads
When a question is free-form (no choices), options is None but it will still serialize as "options": null because the field lacks #[serde(skip_serializing_if = "Option::is_none")]. The request schema and UI logic treat options as an array when present, so null can break JSON-schema validation or any client code that assumes options is iterable. This only impacts questions without options, but it will cause those requests to be rejected or crash UI renderers unless they special‑case null.
Useful? React with 👍 / 👎.
|
@codex review this by comparing it with approvals feature because it blocks also on user response. Mention any missing features, safeguards, or differences between those two implementations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d74863cb6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | EventMsg::ReasoningRawContentDelta(_) | ||
| | EventMsg::RequestUserInput(_) => {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surface request_user_input in TUI instead of ignoring
Unlike exec/file-change approvals, RequestUserInput events are currently dropped in the TUI event loop, so there is no prompt and no way to send Op::RequestUserInputResponse. If the model calls request_user_input (now listed in the toolset when collaboration modes are enabled), the tool call will wait indefinitely and the turn never completes. Approvals have an explicit modal/interrupt path; this new blocking request needs an equivalent UI/response path or should be gated off for TUI clients.
Useful? React with 👍 / 👎.
| Err(err) => { | ||
| error!("request failed: {err:?}"); | ||
| let empty = CoreRequestUserInputResponse { | ||
| answers: HashMap::new(), | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Treat failed user-input requests as cancel/error
The app-server handler synthesizes an empty answers map when the request fails (and also when deserialization fails just below), then submits it as a valid RequestUserInputResponse. In contrast, approvals default to a decline/abort on failure, which prevents actions from proceeding without explicit user input. Returning empty answers here lets the model continue as if the user responded, which can skip required confirmations or choices. Consider returning an error/cancel outcome (or at least a failure flag) instead of silently fabricating an empty response.
Useful? React with 👍 / 👎.
| builder.register_handler("update_plan", plan_handler); | ||
|
|
||
| if config.collaboration_modes_tools { | ||
| builder.push_spec(create_request_user_input_tool()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can do push_spec_with_parallel_support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do want to block on interaction right? haven't 100% looked into the parallel support yet but that would meant mutple process can all trigger userInput requests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think clients should be able to handle that but ok let's not risk it
aibrahim-oai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
056eed0 to
4b15d0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b15d0e581
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | EventMsg::ExitedReviewMode(_) | ||
| | EventMsg::RequestUserInput(_) | ||
| | EventMsg::ContextCompacted(_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle request_user_input in MCP tool runner
When collaboration_modes is enabled and the model calls request_user_input, the core handler blocks waiting for an Op::RequestUserInputResponse. In the MCP tool runner this event is explicitly dropped, so the MCP tools/call session will stall indefinitely after emitting the notification because no response is ever sent back to Codex. This only triggers when the experimental tool is enabled and used, but in that case the tool call never completes. Consider plumbing a request/response path similar to exec approvals or disabling the tool in MCP sessions.
Useful? React with 👍 / 👎.
| | EventMsg::SkillsUpdateAvailable | ||
| | EventMsg::UndoCompleted(_) | ||
| | EventMsg::UndoStarted(_) | ||
| | EventMsg::ThreadRolledBack(_) => {} | ||
| | EventMsg::ThreadRolledBack(_) | ||
| | EventMsg::RequestUserInput(_) => {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surface request_user_input in CLI event processor
The CLI human-output event processor currently drops RequestUserInput events. If collaboration_modes is enabled and the model invokes request_user_input, no prompt is shown and no Op::RequestUserInputResponse can be sent, so the turn will hang waiting on the pending user-input future. This is only hit when the experimental tool is advertised, but it makes the feature unusable in the CLI path until a prompt/response flow is added or the tool is gated off.
Useful? React with 👍 / 👎.
4b15d0e to
697234b
Compare
697234b to
0542051
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0542051958
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let args = RequestUserInputArgs { | ||
| questions: event.questions, | ||
| }; | ||
| let response_fut = | ||
| parent_session.request_user_input(parent_ctx, parent_ctx.sub_id.clone(), args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preserve tool call id when forwarding user input
The delegated handler forwards request_user_input to the parent session using parent_ctx.sub_id as the call_id, which discards the actual tool call id (event.call_id). In collaboration mode, if a delegated agent triggers more than one request_user_input in the same parent turn, each parent-side prompt will share the same item_id and can overwrite/merge in clients that key on item_id, making prompts indistinguishable and responses potentially mapped to the wrong tool call. Use the incoming event.call_id when calling request_user_input so item ids remain unique per tool call.
Useful? React with 👍 / 👎.
| | EventMsg::ItemCompleted(_) | ||
| | EventMsg::AgentMessageContentDelta(_) | ||
| | EventMsg::ReasoningContentDelta(_) | ||
| | EventMsg::ReasoningRawContentDelta(_) => {} | ||
| | EventMsg::ReasoningRawContentDelta(_) | ||
| | EventMsg::RequestUserInput(_) => {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surface request_user_input in TUI2 event loop
The TUI2 chat widget explicitly drops EventMsg::RequestUserInput, so when collaboration modes expose the request_user_input tool and the model calls it, the UI never prompts the user and never sends Op::UserInputAnswer. The tool call future in core will remain pending, so the turn hangs until interrupted. This only affects sessions using the TUI2 frontend and when the tool is invoked, but in those cases the conversation can’t complete.
Useful? React with 👍 / 👎.
Summary
requestUserInputtool that the model can use for gather feedback/asking question mid turn.Tool input schema
Tool output schema