mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
## Why `shell_zsh_fork` already provides stronger guarantees around which executables receive elevated permissions. To reuse that machinery from unified exec without pushing Unix-specific escalation details through generic runtime code, the escalation bootstrap and session lifetime handling need a cleaner boundary. That boundary also needs to be safe for long-lived sessions: when an intercepted shell session is closed or pruned, any in-flight approval workers and any already-approved escalated child they spawned must be torn down with the session, and the inherited escalation socket must not leak into unrelated subprocesses. ## What Changed - Extracted a reusable `EscalationSession` and `EscalateServer::start_session(...)` in `shell-escalation` so callers can get the wrapper/socket env overlay and keep the escalation server alive without immediately running a one-shot command. - Documented that `EscalationSession::env()` and `ShellCommandExecutor::run(...)` exchange only that env overlay, which callers must merge into their own base shell environment. - Clarified the prepared-exec helper boundary in `core` by naming the new helper APIs around `ExecRequest`, while keeping the legacy `execute_env(...)` entrypoints as thin compatibility wrappers for existing callers that still use the older naming. - Added a small post-spawn hook on the prepared execution path so the parent copy of the inheritable escalation socket is closed immediately after both the existing one-shot shell-command spawn and the unified-exec spawn. - Made session teardown explicit with session-scoped cancellation: dropping an `EscalationSession` or canceling its parent request now stops intercept workers, and the server-spawned escalated child uses `kill_on_drop(true)` so teardown cannot orphan an already-approved child. - Added `UnifiedExecBackendConfig` plumbing through `ToolsConfig`, a `shell::zsh_fork_backend` facade, and an opaque unified-exec spawn-lifecycle hook so unified exec can prepare a wrapped `zsh -c/-lc` request without storing `EscalationSession` directly in generic process/runtime code. - Kept the existing `shell_command` zsh-fork behavior intact on top of the new bootstrap path. Tool selection is unchanged in this PR: when `shell_zsh_fork` is enabled, `ShellCommand` still wins over `exec_command`. ## Verification - `cargo test -p codex-shell-escalation` - includes coverage for `start_session_exposes_wrapper_env_overlay` - includes coverage for `exec_closes_parent_socket_after_shell_spawn` - includes coverage for `dropping_session_aborts_intercept_workers_and_kills_spawned_child` - `cargo test -p codex-core shell_zsh_fork_prefers_shell_command_over_unified_exec` - `cargo test -p codex-core --test all shell_zsh_fork_prompts_for_skill_script_execution` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/13392). * #13432 * __->__ #13392
238 lines
6.7 KiB
Rust
238 lines
6.7 KiB
Rust
use std::future::Future;
|
|
use std::sync::Arc;
|
|
use std::time::Duration;
|
|
use std::time::Instant;
|
|
|
|
use tokio::sync::Mutex;
|
|
use tokio::sync::Notify;
|
|
use tokio_util::sync::CancellationToken;
|
|
|
|
#[derive(Clone, Debug)]
|
|
pub struct Stopwatch {
|
|
limit: Option<Duration>,
|
|
inner: Arc<Mutex<StopwatchState>>,
|
|
notify: Arc<Notify>,
|
|
}
|
|
|
|
#[derive(Debug)]
|
|
struct StopwatchState {
|
|
elapsed: Duration,
|
|
running_since: Option<Instant>,
|
|
active_pauses: u32,
|
|
}
|
|
|
|
impl Stopwatch {
|
|
pub fn new(limit: Duration) -> Self {
|
|
Self {
|
|
inner: Arc::new(Mutex::new(StopwatchState {
|
|
elapsed: Duration::ZERO,
|
|
running_since: Some(Instant::now()),
|
|
active_pauses: 0,
|
|
})),
|
|
notify: Arc::new(Notify::new()),
|
|
limit: Some(limit),
|
|
}
|
|
}
|
|
|
|
pub fn unlimited() -> Self {
|
|
Self {
|
|
inner: Arc::new(Mutex::new(StopwatchState {
|
|
elapsed: Duration::ZERO,
|
|
running_since: Some(Instant::now()),
|
|
active_pauses: 0,
|
|
})),
|
|
notify: Arc::new(Notify::new()),
|
|
limit: None,
|
|
}
|
|
}
|
|
|
|
pub fn cancellation_token(&self) -> CancellationToken {
|
|
let token = CancellationToken::new();
|
|
let Some(limit) = self.limit else {
|
|
return token;
|
|
};
|
|
let cancel = token.clone();
|
|
let inner = Arc::clone(&self.inner);
|
|
let notify = Arc::clone(&self.notify);
|
|
tokio::spawn(async move {
|
|
loop {
|
|
let (remaining, running) = {
|
|
let guard = inner.lock().await;
|
|
let elapsed = guard.elapsed
|
|
+ guard
|
|
.running_since
|
|
.map(|since| since.elapsed())
|
|
.unwrap_or_default();
|
|
if elapsed >= limit {
|
|
break;
|
|
}
|
|
(limit - elapsed, guard.running_since.is_some())
|
|
};
|
|
|
|
if !running {
|
|
notify.notified().await;
|
|
continue;
|
|
}
|
|
|
|
let sleep = tokio::time::sleep(remaining);
|
|
tokio::pin!(sleep);
|
|
tokio::select! {
|
|
_ = &mut sleep => {
|
|
break;
|
|
}
|
|
_ = notify.notified() => {
|
|
continue;
|
|
}
|
|
}
|
|
}
|
|
cancel.cancel();
|
|
});
|
|
token
|
|
}
|
|
|
|
/// Runs `fut`, pausing the stopwatch while the future is pending. The clock
|
|
/// resumes automatically when the future completes. Nested/overlapping
|
|
/// calls are reference-counted so the stopwatch only resumes when every
|
|
/// pause is lifted.
|
|
pub async fn pause_for<F, T>(&self, fut: F) -> T
|
|
where
|
|
F: Future<Output = T>,
|
|
{
|
|
self.pause().await;
|
|
let result = fut.await;
|
|
self.resume().await;
|
|
result
|
|
}
|
|
|
|
async fn pause(&self) {
|
|
let mut guard = self.inner.lock().await;
|
|
guard.active_pauses += 1;
|
|
if guard.active_pauses == 1
|
|
&& let Some(since) = guard.running_since.take()
|
|
{
|
|
guard.elapsed += since.elapsed();
|
|
self.notify.notify_waiters();
|
|
}
|
|
}
|
|
|
|
async fn resume(&self) {
|
|
let mut guard = self.inner.lock().await;
|
|
if guard.active_pauses == 0 {
|
|
return;
|
|
}
|
|
guard.active_pauses -= 1;
|
|
if guard.active_pauses == 0 && guard.running_since.is_none() {
|
|
guard.running_since = Some(Instant::now());
|
|
self.notify.notify_waiters();
|
|
}
|
|
}
|
|
}
|
|
|
|
#[cfg(test)]
|
|
mod tests {
|
|
use super::Stopwatch;
|
|
use tokio::time::Duration;
|
|
use tokio::time::Instant;
|
|
use tokio::time::sleep;
|
|
use tokio::time::timeout;
|
|
|
|
#[tokio::test]
|
|
async fn cancellation_receiver_fires_after_limit() {
|
|
let stopwatch = Stopwatch::new(Duration::from_millis(50));
|
|
let token = stopwatch.cancellation_token();
|
|
let start = Instant::now();
|
|
token.cancelled().await;
|
|
assert!(start.elapsed() >= Duration::from_millis(50));
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn pause_prevents_timeout_until_resumed() {
|
|
let stopwatch = Stopwatch::new(Duration::from_millis(50));
|
|
let token = stopwatch.cancellation_token();
|
|
|
|
let pause_handle = tokio::spawn({
|
|
let stopwatch = stopwatch.clone();
|
|
async move {
|
|
stopwatch
|
|
.pause_for(async {
|
|
sleep(Duration::from_millis(100)).await;
|
|
})
|
|
.await;
|
|
}
|
|
});
|
|
|
|
assert!(
|
|
timeout(Duration::from_millis(30), token.cancelled())
|
|
.await
|
|
.is_err()
|
|
);
|
|
|
|
pause_handle.await.expect("pause task should finish");
|
|
|
|
token.cancelled().await;
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn overlapping_pauses_only_resume_once() {
|
|
let stopwatch = Stopwatch::new(Duration::from_millis(50));
|
|
let token = stopwatch.cancellation_token();
|
|
|
|
// First pause.
|
|
let pause1 = {
|
|
let stopwatch = stopwatch.clone();
|
|
tokio::spawn(async move {
|
|
stopwatch
|
|
.pause_for(async {
|
|
sleep(Duration::from_millis(80)).await;
|
|
})
|
|
.await;
|
|
})
|
|
};
|
|
|
|
// Overlapping pause that ends sooner.
|
|
let pause2 = {
|
|
let stopwatch = stopwatch.clone();
|
|
tokio::spawn(async move {
|
|
stopwatch
|
|
.pause_for(async {
|
|
sleep(Duration::from_millis(30)).await;
|
|
})
|
|
.await;
|
|
})
|
|
};
|
|
|
|
// While both pauses are active, the cancellation should not fire.
|
|
assert!(
|
|
timeout(Duration::from_millis(40), token.cancelled())
|
|
.await
|
|
.is_err()
|
|
);
|
|
|
|
pause2.await.expect("short pause should complete");
|
|
|
|
// Still paused because the long pause is active.
|
|
assert!(
|
|
timeout(Duration::from_millis(30), token.cancelled())
|
|
.await
|
|
.is_err()
|
|
);
|
|
|
|
pause1.await.expect("long pause should complete");
|
|
|
|
// Now the stopwatch should resume and hit the limit shortly after.
|
|
token.cancelled().await;
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn unlimited_stopwatch_never_cancels() {
|
|
let stopwatch = Stopwatch::unlimited();
|
|
let token = stopwatch.cancellation_token();
|
|
|
|
assert!(
|
|
timeout(Duration::from_millis(30), token.cancelled())
|
|
.await
|
|
.is_err()
|
|
);
|
|
}
|
|
}
|