mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Remove no-tool goal continuation suppression (#20523)
## Why `/goal` is supposed to keep Codex working until the goal is actually done. The previous continuation logic had two ways to stop early: the continuation prompt told the model to wait for new input when it felt blocked, and the runtime suppressed another continuation turn after a continuation finished without any tool calls. That made goals stop short even when the agent could still keep making progress (I received a few reports of this from users). It also relied on a brittle heuristic that treated "no registry tool calls" as equivalent to "should stop." ## What changed - removed the continuation prompt sentence that told the model to stop and wait for new input when it could not continue productively - removed the goal runtime suppression heuristic that stopped auto-continuation after a no-tool continuation turn - deleted the continuation-activity bookkeeping and left `tool_calls` as telemetry only - added focused regressions for the two intended behaviors: completed no-tool continuation turns still continue, while `request_user_input` keeps the existing turn open instead of spawning a new continuation
This commit is contained in:
@@ -29,8 +29,6 @@ use codex_utils_template::Template;
|
||||
use futures::future::BoxFuture;
|
||||
use std::sync::Arc;
|
||||
use std::sync::LazyLock;
|
||||
use std::sync::atomic::AtomicBool;
|
||||
use std::sync::atomic::Ordering;
|
||||
use std::time::Duration;
|
||||
use std::time::Instant;
|
||||
use tokio::sync::Mutex;
|
||||
@@ -90,7 +88,6 @@ pub(crate) enum GoalRuntimeEvent<'a> {
|
||||
TurnFinished {
|
||||
turn_context: &'a TurnContext,
|
||||
turn_completed: bool,
|
||||
tool_calls: u64,
|
||||
},
|
||||
MaybeContinueIfIdle,
|
||||
TaskAborted {
|
||||
@@ -112,7 +109,6 @@ pub(crate) struct GoalRuntimeState {
|
||||
accounting: Mutex<GoalAccountingSnapshot>,
|
||||
continuation_turn_id: Mutex<Option<String>>,
|
||||
pub(crate) continuation_lock: Semaphore,
|
||||
pub(crate) continuation_suppressed: AtomicBool,
|
||||
}
|
||||
|
||||
struct GoalContinuationCandidate {
|
||||
@@ -129,7 +125,6 @@ impl GoalRuntimeState {
|
||||
accounting: Mutex::new(GoalAccountingSnapshot::new()),
|
||||
continuation_turn_id: Mutex::new(None),
|
||||
continuation_lock: Semaphore::new(/*permits*/ 1),
|
||||
continuation_suppressed: AtomicBool::new(false),
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -277,8 +272,8 @@ impl Session {
|
||||
/// suppresses that steering, external mutations account best-effort before
|
||||
/// changing state, interrupts pause active goals, resumes reactivate paused
|
||||
/// goals, explicit maybe-continue events start idle goal continuation turns,
|
||||
/// and no-tool continuation turns suppress the next automatic continuation
|
||||
/// until user/tool/external activity resets it.
|
||||
/// and continuation turns with no counted autonomous activity suppress the
|
||||
/// next automatic continuation until user/tool/external activity resets it.
|
||||
pub(crate) fn goal_runtime_apply<'a>(
|
||||
self: &'a Arc<Self>,
|
||||
event: GoalRuntimeEvent<'a>,
|
||||
@@ -296,7 +291,6 @@ impl Session {
|
||||
turn_context,
|
||||
tool_name,
|
||||
} => Box::pin(async move {
|
||||
self.reset_thread_goal_continuation_suppression();
|
||||
if tool_name != codex_tools::UPDATE_GOAL_TOOL_NAME {
|
||||
self.account_thread_goal_progress(turn_context, BudgetLimitSteering::Allowed)
|
||||
.await?;
|
||||
@@ -304,7 +298,6 @@ impl Session {
|
||||
Ok(())
|
||||
}),
|
||||
GoalRuntimeEvent::ToolCompletedGoal { turn_context } => Box::pin(async move {
|
||||
self.reset_thread_goal_continuation_suppression();
|
||||
self.account_thread_goal_progress(turn_context, BudgetLimitSteering::Suppressed)
|
||||
.await?;
|
||||
Ok(())
|
||||
@@ -312,9 +305,8 @@ impl Session {
|
||||
GoalRuntimeEvent::TurnFinished {
|
||||
turn_context,
|
||||
turn_completed,
|
||||
tool_calls,
|
||||
} => Box::pin(async move {
|
||||
self.finish_thread_goal_turn(turn_context, turn_completed, tool_calls)
|
||||
self.finish_thread_goal_turn(turn_context, turn_completed)
|
||||
.await;
|
||||
Ok(())
|
||||
}),
|
||||
@@ -331,7 +323,6 @@ impl Session {
|
||||
Ok(())
|
||||
}),
|
||||
GoalRuntimeEvent::ExternalMutationStarting => Box::pin(async move {
|
||||
self.reset_thread_goal_continuation_suppression();
|
||||
if let Err(err) = self.account_thread_goal_before_external_mutation().await {
|
||||
tracing::warn!(
|
||||
"failed to account thread goal progress before external mutation: {err}"
|
||||
@@ -463,7 +454,6 @@ impl Session {
|
||||
let goal_status = goal.status;
|
||||
let goal_id = goal.goal_id.clone();
|
||||
let goal = protocol_goal_from_state(goal);
|
||||
self.reset_thread_goal_continuation_suppression();
|
||||
*self.goal_runtime.budget_limit_reported_goal_id.lock().await = None;
|
||||
let newly_active_goal = goal_status == codex_state::ThreadGoalStatus::Active
|
||||
&& (replacing_goal
|
||||
@@ -532,7 +522,6 @@ impl Session {
|
||||
|
||||
let goal_id = goal.goal_id.clone();
|
||||
let goal = protocol_goal_from_state(goal);
|
||||
self.reset_thread_goal_continuation_suppression();
|
||||
*self.goal_runtime.budget_limit_reported_goal_id.lock().await = None;
|
||||
|
||||
let current_token_usage = self.total_token_usage().await.unwrap_or_default();
|
||||
@@ -561,7 +550,6 @@ impl Session {
|
||||
) {
|
||||
match status {
|
||||
codex_state::ThreadGoalStatus::Active => {
|
||||
self.reset_thread_goal_continuation_suppression();
|
||||
match self.state_db_for_thread_goals().await {
|
||||
Ok(Some(state_db)) => {
|
||||
match state_db.get_thread_goal(self.conversation_id).await {
|
||||
@@ -608,7 +596,6 @@ impl Session {
|
||||
}
|
||||
|
||||
async fn clear_stopped_thread_goal_runtime_state(&self) {
|
||||
self.reset_thread_goal_continuation_suppression();
|
||||
*self.goal_runtime.budget_limit_reported_goal_id.lock().await = None;
|
||||
let mut accounting = self.goal_runtime.accounting.lock().await;
|
||||
if let Some(turn) = accounting.turn.as_mut() {
|
||||
@@ -663,16 +650,6 @@ impl Session {
|
||||
turn_context: &TurnContext,
|
||||
token_usage: TokenUsage,
|
||||
) {
|
||||
if self
|
||||
.goal_runtime
|
||||
.continuation_turn_id
|
||||
.lock()
|
||||
.await
|
||||
.as_ref()
|
||||
.is_none_or(|turn_id| turn_id != &turn_context.sub_id)
|
||||
{
|
||||
self.reset_thread_goal_continuation_suppression();
|
||||
}
|
||||
self.goal_runtime.accounting.lock().await.turn = Some(GoalTurnAccountingSnapshot::new(
|
||||
turn_context.sub_id.clone(),
|
||||
token_usage,
|
||||
@@ -723,12 +700,6 @@ impl Session {
|
||||
}
|
||||
}
|
||||
|
||||
fn reset_thread_goal_continuation_suppression(&self) {
|
||||
self.goal_runtime
|
||||
.continuation_suppressed
|
||||
.store(false, Ordering::SeqCst);
|
||||
}
|
||||
|
||||
async fn mark_thread_goal_continuation_turn_started(&self, turn_id: String) {
|
||||
*self.goal_runtime.continuation_turn_id.lock().await = Some(turn_id);
|
||||
}
|
||||
@@ -757,7 +728,6 @@ impl Session {
|
||||
self: &Arc<Self>,
|
||||
turn_context: &TurnContext,
|
||||
turn_completed: bool,
|
||||
turn_tool_calls: u64,
|
||||
) {
|
||||
if turn_completed
|
||||
&& let Err(err) = self
|
||||
@@ -767,15 +737,8 @@ impl Session {
|
||||
tracing::warn!("failed to account thread goal progress at turn end: {err}");
|
||||
}
|
||||
|
||||
if self
|
||||
.take_thread_goal_continuation_turn(&turn_context.sub_id)
|
||||
.await
|
||||
&& turn_tool_calls == 0
|
||||
{
|
||||
self.goal_runtime
|
||||
.continuation_suppressed
|
||||
.store(true, Ordering::SeqCst);
|
||||
}
|
||||
self.take_thread_goal_continuation_turn(&turn_context.sub_id)
|
||||
.await;
|
||||
if turn_completed {
|
||||
let mut accounting = self.goal_runtime.accounting.lock().await;
|
||||
if accounting
|
||||
@@ -1126,7 +1089,6 @@ impl Session {
|
||||
};
|
||||
let goal_id = goal.goal_id.clone();
|
||||
let goal = protocol_goal_from_state(goal);
|
||||
self.reset_thread_goal_continuation_suppression();
|
||||
*self.goal_runtime.budget_limit_reported_goal_id.lock().await = None;
|
||||
let active_turn_id = self
|
||||
.active_turn_context()
|
||||
@@ -1255,16 +1217,6 @@ impl Session {
|
||||
);
|
||||
return None;
|
||||
}
|
||||
if self
|
||||
.goal_runtime
|
||||
.continuation_suppressed
|
||||
.load(Ordering::SeqCst)
|
||||
{
|
||||
tracing::debug!(
|
||||
"skipping active goal continuation because the last continuation made no tool calls"
|
||||
);
|
||||
return None;
|
||||
}
|
||||
let state_db = match self.state_db_for_thread_goals().await {
|
||||
Ok(Some(state_db)) => state_db,
|
||||
Ok(None) => {
|
||||
@@ -1578,7 +1530,7 @@ mod tests {
|
||||
assert!(prompt.contains("<untrusted_objective>\nfinish the stack\n</untrusted_objective>"));
|
||||
assert!(prompt.contains("Token budget: 10000"));
|
||||
assert!(prompt.contains("call update_goal with status \"complete\""));
|
||||
assert!(prompt.contains(
|
||||
assert!(!prompt.contains(
|
||||
"explain the blocker or next required input to the user and wait for new input"
|
||||
));
|
||||
assert!(!prompt.contains("budgetLimited"));
|
||||
|
||||
@@ -118,6 +118,8 @@ use codex_protocol::protocol::TurnCompleteEvent;
|
||||
use codex_protocol::protocol::TurnStartedEvent;
|
||||
use codex_protocol::protocol::UserMessageEvent;
|
||||
use codex_protocol::protocol::W3cTraceContext;
|
||||
use codex_protocol::request_user_input::RequestUserInputAnswer;
|
||||
use codex_protocol::request_user_input::RequestUserInputResponse;
|
||||
use core_test_support::PathBufExt;
|
||||
use core_test_support::PathExt;
|
||||
use core_test_support::context_snapshot;
|
||||
@@ -136,6 +138,7 @@ use core_test_support::test_codex::test_codex;
|
||||
use core_test_support::test_path_buf;
|
||||
use core_test_support::tracing::install_test_tracing;
|
||||
use core_test_support::wait_for_event;
|
||||
use core_test_support::wait_for_event_match;
|
||||
use opentelemetry::trace::TraceContextExt;
|
||||
use opentelemetry::trace::TraceId;
|
||||
use opentelemetry_sdk::metrics::InMemoryMetricExporter;
|
||||
@@ -6958,7 +6961,7 @@ async fn interrupt_accounts_active_goal_before_pausing() -> anyhow::Result<()> {
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn active_goal_continuation_runs_to_completion_after_turn() -> anyhow::Result<()> {
|
||||
async fn active_goal_continuation_runs_again_after_no_tool_turn() -> anyhow::Result<()> {
|
||||
let server = start_mock_server().await;
|
||||
let mut builder = test_codex().with_config(|config| {
|
||||
config
|
||||
@@ -6984,17 +6987,21 @@ async fn active_goal_continuation_runs_to_completion_after_turn() -> anyhow::Res
|
||||
ev_completed("resp-2"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-3"),
|
||||
ev_assistant_message("msg-2", "I am still working on the benchmark note."),
|
||||
ev_completed("resp-3"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-4"),
|
||||
ev_function_call(
|
||||
"call-complete-goal",
|
||||
"update_goal",
|
||||
r#"{"status":"complete"}"#,
|
||||
),
|
||||
ev_completed("resp-3"),
|
||||
ev_completed("resp-4"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_assistant_message("msg-2", "Goal complete."),
|
||||
ev_completed("resp-4"),
|
||||
ev_assistant_message("msg-3", "Goal complete."),
|
||||
ev_completed("resp-5"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
@@ -7018,7 +7025,7 @@ async fn active_goal_continuation_runs_to_completion_after_turn() -> anyhow::Res
|
||||
let event = test.codex.next_event().await?;
|
||||
if matches!(event.msg, EventMsg::TurnComplete(_)) {
|
||||
completed_turns += 1;
|
||||
if completed_turns == 2 {
|
||||
if completed_turns == 3 {
|
||||
return anyhow::Ok(());
|
||||
}
|
||||
}
|
||||
@@ -7029,6 +7036,125 @@ async fn active_goal_continuation_runs_to_completion_after_turn() -> anyhow::Res
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn pending_request_user_input_does_not_spawn_extra_goal_continuation() -> anyhow::Result<()> {
|
||||
let server = start_mock_server().await;
|
||||
let mut builder = test_codex().with_config(|config| {
|
||||
config
|
||||
.features
|
||||
.enable(Feature::Goals)
|
||||
.expect("goal mode should be enableable in tests");
|
||||
config
|
||||
.features
|
||||
.enable(Feature::DefaultModeRequestUserInput)
|
||||
.expect("default-mode request_user_input should be enableable in tests");
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
let responses = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-1"),
|
||||
ev_function_call(
|
||||
"call-create-goal",
|
||||
"create_goal",
|
||||
r#"{"objective":"write a benchmark note"}"#,
|
||||
),
|
||||
ev_completed("resp-1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_assistant_message("msg-1", "Draft ready."),
|
||||
ev_completed("resp-2"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-3"),
|
||||
ev_function_call(
|
||||
"call-ask-user",
|
||||
"request_user_input",
|
||||
r#"{"questions":[{"header":"Choice","id":"next_step","question":"Pick one","options":[{"label":"Outline","description":"Start with an outline."},{"label":"Draft","description":"Write a full draft."}]}]}"#,
|
||||
),
|
||||
ev_completed("resp-3"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-4"),
|
||||
ev_function_call(
|
||||
"call-complete-goal",
|
||||
"update_goal",
|
||||
r#"{"status":"complete"}"#,
|
||||
),
|
||||
ev_completed("resp-4"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_assistant_message("msg-2", "Goal complete."),
|
||||
ev_completed("resp-5"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
test.codex
|
||||
.submit(Op::UserInput {
|
||||
environments: None,
|
||||
items: vec![UserInput::Text {
|
||||
text: "write a benchmark note".into(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
final_output_json_schema: None,
|
||||
responsesapi_client_metadata: None,
|
||||
})
|
||||
.await?;
|
||||
|
||||
let request_user_input_event = wait_for_event_match(&test.codex, |event| match event {
|
||||
EventMsg::RequestUserInput(event) => Some(event.clone()),
|
||||
_ => None,
|
||||
})
|
||||
.await;
|
||||
assert_eq!(3, responses.requests().len());
|
||||
assert!(
|
||||
timeout(Duration::from_millis(200), test.codex.next_event())
|
||||
.await
|
||||
.is_err(),
|
||||
"waiting for request_user_input should keep the turn open without emitting more events"
|
||||
);
|
||||
assert_eq!(
|
||||
3,
|
||||
responses.requests().len(),
|
||||
"waiting for request_user_input should not start another continuation request"
|
||||
);
|
||||
|
||||
test.codex
|
||||
.submit(Op::UserInputAnswer {
|
||||
id: request_user_input_event.turn_id,
|
||||
response: RequestUserInputResponse {
|
||||
answers: std::collections::HashMap::from([(
|
||||
"next_step".to_string(),
|
||||
RequestUserInputAnswer {
|
||||
answers: vec!["Outline".to_string()],
|
||||
},
|
||||
)]),
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
|
||||
let mut completed_turns = 0;
|
||||
timeout(Duration::from_secs(8), async {
|
||||
loop {
|
||||
let event = test.codex.next_event().await?;
|
||||
if matches!(event.msg, EventMsg::TurnComplete(_)) {
|
||||
completed_turns += 1;
|
||||
if completed_turns == 1 {
|
||||
return anyhow::Ok(());
|
||||
}
|
||||
}
|
||||
}
|
||||
})
|
||||
.await??;
|
||||
|
||||
assert_eq!(5, responses.requests().len());
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn set_total_token_usage(sess: &Session, total_token_usage: TokenUsage) {
|
||||
let mut state = sess.state.lock().await;
|
||||
state.set_token_info(Some(TokenUsageInfo {
|
||||
|
||||
@@ -255,14 +255,14 @@ pub(crate) async fn handle_output_item_done(
|
||||
}
|
||||
// No tool call: convert messages/reasoning into turn items and mark them as complete.
|
||||
Ok(None) => {
|
||||
if let Some(turn_item) = handle_non_tool_response_item(
|
||||
let turn_item = handle_non_tool_response_item(
|
||||
ctx.sess.as_ref(),
|
||||
ctx.turn_context.as_ref(),
|
||||
&item,
|
||||
plan_mode,
|
||||
)
|
||||
.await
|
||||
{
|
||||
.await;
|
||||
if let Some(turn_item) = turn_item {
|
||||
if previously_active_item.is_none() {
|
||||
let mut started_item = turn_item.clone();
|
||||
if let TurnItem::ImageGeneration(item) = &mut started_item {
|
||||
|
||||
@@ -735,7 +735,6 @@ impl Session {
|
||||
.goal_runtime_apply(GoalRuntimeEvent::TurnFinished {
|
||||
turn_context: turn_context.as_ref(),
|
||||
turn_completed: should_clear_active_turn,
|
||||
tool_calls: turn_tool_calls,
|
||||
})
|
||||
.await
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user