mirror of
https://github.com/openai/codex.git
synced 2026-05-03 04:42:20 +03:00
feat: waiting for an elicitation should not count against a shell tool timeout (#6973)
Previously, we were running into an issue where we would run the `shell` tool call with a timeout of 10s, but it fired an elicitation asking for user approval, the time the user took to respond to the elicitation was counted agains the 10s timeout, so the `shell` tool call would fail with a timeout error unless the user is very fast! This PR addresses this issue by introducing a "stopwatch" abstraction that is used to manage the timeout. The idea is: - `Stopwatch::new()` is called with the _real_ timeout of the `shell` tool call. - `process_exec_tool_call()` is called with the `Cancellation` variant of `ExecExpiration` because it should not manage its own timeout in this case - the `Stopwatch` expiration is wired up to the `cancel_rx` passed to `process_exec_tool_call()` - when an elicitation for the `shell` tool call is received, the `Stopwatch` pauses - because it is possible for multiple elicitations to arrive concurrently, it keeps track of the number of "active pauses" and does not resume until that counter goes down to zero I verified that I can test the MCP server using `@modelcontextprotocol/inspector` and specify `git status` as the `command` with a timeout of 500ms and that the elicitation pops up and I have all the time in the world to respond whereas previous to this PR, that would not have been possible. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/6973). * #7005 * __->__ #6973 * #6972
This commit is contained in:
@@ -10,6 +10,7 @@ use rmcp::service::RequestContext;
|
||||
|
||||
use crate::posix::escalate_protocol::EscalateAction;
|
||||
use crate::posix::escalation_policy::EscalationPolicy;
|
||||
use crate::posix::stopwatch::Stopwatch;
|
||||
|
||||
/// This is the policy which decides how to handle an exec() call.
|
||||
///
|
||||
@@ -34,11 +35,20 @@ pub(crate) enum ExecPolicyOutcome {
|
||||
pub(crate) struct McpEscalationPolicy {
|
||||
policy: ExecPolicy,
|
||||
context: RequestContext<RoleServer>,
|
||||
stopwatch: Stopwatch,
|
||||
}
|
||||
|
||||
impl McpEscalationPolicy {
|
||||
pub(crate) fn new(policy: ExecPolicy, context: RequestContext<RoleServer>) -> Self {
|
||||
Self { policy, context }
|
||||
pub(crate) fn new(
|
||||
policy: ExecPolicy,
|
||||
context: RequestContext<RoleServer>,
|
||||
stopwatch: Stopwatch,
|
||||
) -> Self {
|
||||
Self {
|
||||
policy,
|
||||
context,
|
||||
stopwatch,
|
||||
}
|
||||
}
|
||||
|
||||
async fn prompt(
|
||||
@@ -54,25 +64,34 @@ impl McpEscalationPolicy {
|
||||
} else {
|
||||
format!("{} {}", file.display(), args)
|
||||
};
|
||||
context
|
||||
.peer
|
||||
.create_elicitation(CreateElicitationRequestParam {
|
||||
message: format!("Allow agent to run `{command}` in `{}`?", workdir.display()),
|
||||
requested_schema: ElicitationSchema::builder()
|
||||
.title("Execution Permission Request")
|
||||
.optional_string_with("reason", |schema| {
|
||||
schema.description("Optional reason for allowing or denying execution")
|
||||
self.stopwatch
|
||||
.pause_for(async {
|
||||
context
|
||||
.peer
|
||||
.create_elicitation(CreateElicitationRequestParam {
|
||||
message: format!(
|
||||
"Allow agent to run `{command}` in `{}`?",
|
||||
workdir.display()
|
||||
),
|
||||
requested_schema: ElicitationSchema::builder()
|
||||
.title("Execution Permission Request")
|
||||
.optional_string_with("reason", |schema| {
|
||||
schema.description(
|
||||
"Optional reason for allowing or denying execution",
|
||||
)
|
||||
})
|
||||
.build()
|
||||
.map_err(|e| {
|
||||
McpError::internal_error(
|
||||
format!("failed to build elicitation schema: {e}"),
|
||||
None,
|
||||
)
|
||||
})?,
|
||||
})
|
||||
.build()
|
||||
.map_err(|e| {
|
||||
McpError::internal_error(
|
||||
format!("failed to build elicitation schema: {e}"),
|
||||
None,
|
||||
)
|
||||
})?,
|
||||
.await
|
||||
.map_err(|e| McpError::internal_error(e.to_string(), None))
|
||||
})
|
||||
.await
|
||||
.map_err(|e| McpError::internal_error(e.to_string(), None))
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user