Skip to content

add branch name w/ sync function for git info #2142

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions codex-rs/core/src/codex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -902,9 +902,11 @@ async fn submission_loop(
}
}

// Gather history metadata for SessionConfiguredEvent.
let (history_log_id, history_entry_count) =
crate::message_history::history_metadata(&config).await;
// Gather history metadata for SessionConfiguredEvent and await git info.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we should be specifying the timeout somehow at this callsite (either updating collect_git_info() to take a timeout, or wrapping these calls with tokio::time::timeout) to ensure that this runs in a bounded amount of time.

One challenge is that here, we are latency-sensitive and I agre with the short timeout.

Though the other callsite in rollout.rs is less latency-sensitive, and arguably should be able to specify a more generous timeout.

That said, from the callsite, one would expect to specify the timeout of the overall operation, whereas internally inside collect_git_info(), run_git_command_with_timeout() is run twice in series, so if collect_git_info() took its own timeout argument, arguably we should divide it by two and use the halved version for each internal call to run_git_command_with_timeout()?

I admit there's already the existing issue here that crate::message_history::history_metadata() should also be bound by a timeout, though I don't know what value to use (0?) if the timeout expires before it completes.

let git_info_fut = crate::git_info::collect_git_info(&sess.as_ref().unwrap().cwd);
let history_fut = crate::message_history::history_metadata(&config);
let (git_info, (history_log_id, history_entry_count)) =
tokio::join!(git_info_fut, history_fut);

// ack
let events = std::iter::once(Event {
Expand All @@ -914,6 +916,7 @@ async fn submission_loop(
model,
history_log_id,
history_entry_count,
git_info,
}),
})
.chain(mcp_connection_errors.into_iter());
Expand Down
8 changes: 6 additions & 2 deletions codex-rs/core/src/git_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,12 @@ use tokio::process::Command;
use tokio::time::Duration as TokioDuration;
use tokio::time::timeout;

/// Timeout for git commands to prevent freezing on large repositories
const GIT_COMMAND_TIMEOUT: TokioDuration = TokioDuration::from_secs(5);
/// Timeout for git commands to prevent freezing on large repositories.
///
/// Tests that wait for the initial `SessionConfigured` event use a short
/// timeout (~1s). Collecting Git info is best-effort and must not block the
/// session handshake, so we cap individual Git calls to a small value.
const GIT_COMMAND_TIMEOUT: TokioDuration = TokioDuration::from_millis(400);

#[derive(Serialize, Deserialize, Clone, Debug)]
pub struct GitInfo {
Expand Down
6 changes: 6 additions & 0 deletions codex-rs/core/src/protocol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use uuid::Uuid;

use crate::config_types::ReasoningEffort as ReasoningEffortConfig;
use crate::config_types::ReasoningSummary as ReasoningSummaryConfig;
use crate::git_info::GitInfo;
use crate::message_history::HistoryEntry;
use crate::model_provider_info::ModelProviderInfo;
use crate::parse_command::ParsedCommand;
Expand Down Expand Up @@ -695,6 +696,10 @@ pub struct SessionConfiguredEvent {

/// Current number of entries in the history log.
pub history_entry_count: usize,

/// Optional Git metadata for the configured cwd.
#[serde(skip_serializing_if = "Option::is_none")]
pub git_info: Option<GitInfo>,
}

/// User's decision in response to an ExecApprovalRequest.
Expand Down Expand Up @@ -757,6 +762,7 @@ mod tests {
model: "codex-mini-latest".to_string(),
history_log_id: 0,
history_entry_count: 0,
git_info: None,
}),
};
let serialized = serde_json::to_string(&event).unwrap();
Expand Down
1 change: 1 addition & 0 deletions codex-rs/exec/src/event_processor_with_human_output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,7 @@ impl EventProcessor for EventProcessorWithHumanOutput {
model,
history_log_id: _,
history_entry_count: _,
git_info: _,
} = session_configured_event;

ts_println!(
Expand Down
1 change: 1 addition & 0 deletions codex-rs/mcp-server/src/mcp_protocol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -908,6 +908,7 @@ mod tests {
model: "codex-mini-latest".into(),
history_log_id: 42,
history_entry_count: 3,
git_info: None,
}),
};

Expand Down
2 changes: 2 additions & 0 deletions codex-rs/mcp-server/src/outgoing_message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ mod tests {
model: "gpt-4o".to_string(),
history_log_id: 1,
history_entry_count: 1000,
git_info: None,
}),
};

Expand Down Expand Up @@ -284,6 +285,7 @@ mod tests {
model: "gpt-4o".to_string(),
history_log_id: 1,
history_entry_count: 1000,
git_info: None,
};
let event = Event {
id: "1".to_string(),
Expand Down
17 changes: 16 additions & 1 deletion codex-rs/tui/src/history_cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ impl HistoryCell {
session_id: _,
history_log_id: _,
history_entry_count: _,
git_info,
} = event;
if is_first_event {
let cwd_str = match relativize_to_home(&config.cwd) {
Expand All @@ -241,14 +242,26 @@ impl HistoryCell {
None => config.cwd.display().to_string(),
};

// Use async-collected Git info from the event if available.
let branch_suffix = git_info
.and_then(|g| g.branch)
.map(|b| format!(" ({b})"))
.unwrap_or_default();

let path_and_branch = if branch_suffix.is_empty() {
format!(" {cwd_str}")
} else {
format!(" {cwd_str}{branch_suffix}")
};

let lines: Vec<Line<'static>> = vec![
Line::from(vec![
Span::raw(">_ ").dim(),
Span::styled(
"You are using OpenAI Codex in",
Style::default().add_modifier(Modifier::BOLD),
),
Span::raw(format!(" {cwd_str}")).dim(),
Span::raw(path_and_branch).dim(),
]),
Line::from("".dim()),
Line::from(" To get started, describe a task or try one of these commands:".dim()),
Expand Down Expand Up @@ -883,6 +896,8 @@ impl HistoryCell {
}
}

//
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove?


impl WidgetRef for &HistoryCell {
fn render_ref(&self, area: Rect, buf: &mut Buffer) {
Paragraph::new(Text::from(self.plain_lines()))
Expand Down
Loading