Skip to content
Merged
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
28 changes: 26 additions & 2 deletions codex-rs/app-server/src/codex_message_processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,7 @@ use codex_features::FEATURES;
use codex_features::Feature;
use codex_features::Stage;
use codex_feedback::CodexFeedback;
use codex_feedback::FeedbackAttachmentPath;
use codex_feedback::FeedbackUploadOptions;
use codex_git_utils::git_diff_to_remote;
use codex_git_utils::resolve_root_git_project_for_trust;
Expand Down Expand Up @@ -7733,14 +7734,33 @@ impl CodexMessageProcessor {
continue;
};
if seen_attachment_paths.insert(rollout_path.clone()) {
attachment_paths.push(rollout_path);
attachment_paths.push(FeedbackAttachmentPath {
path: rollout_path,
attachment_filename_override: None,
});
}
}
if let Some(conversation_id) = conversation_id
&& let Ok(conversation) = self.thread_manager.get_thread(conversation_id).await
&& let Some(guardian_rollout_path) =
conversation.guardian_trunk_rollout_path().await
&& seen_attachment_paths.insert(guardian_rollout_path.clone())
{
attachment_paths.push(FeedbackAttachmentPath {
path: guardian_rollout_path,
attachment_filename_override: Some(auto_review_rollout_filename(
conversation_id,
)),
});
}
}
if let Some(extra_log_files) = extra_log_files {
for extra_log_file in extra_log_files {
if seen_attachment_paths.insert(extra_log_file.clone()) {
attachment_paths.push(extra_log_file);
attachment_paths.push(FeedbackAttachmentPath {
path: extra_log_file,
attachment_filename_override: None,
});
}
}
}
Expand Down Expand Up @@ -7885,6 +7905,10 @@ impl CodexMessageProcessor {
}
}

fn auto_review_rollout_filename(thread_id: ThreadId) -> String {
format!("auto-review-rollout-{thread_id}.jsonl")
}

fn normalize_thread_list_cwd_filters(
cwd: Option<ThreadListCwdFilter>,
) -> Result<Option<Vec<PathBuf>>, JSONRPCErrorError> {
Expand Down
8 changes: 8 additions & 0 deletions codex-rs/core/src/codex_thread.rs
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,14 @@ impl CodexThread {
self.rollout_path.clone()
}

pub async fn guardian_trunk_rollout_path(&self) -> Option<PathBuf> {
self.codex
.session
.guardian_review_session
.trunk_rollout_path()
.await
}

pub fn state_db(&self) -> Option<StateDbHandle> {
self.codex.state_db()
}
Expand Down
12 changes: 12 additions & 0 deletions codex-rs/core/src/guardian/review_session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,18 @@ impl Drop for EphemeralReviewCleanup {
}

impl GuardianReviewSessionManager {
pub(crate) async fn trunk_rollout_path(&self) -> Option<PathBuf> {
let trunk = self.state.lock().await.trunk.clone()?;
trunk.codex.session.ensure_rollout_materialized().await;
match trunk.codex.session.current_rollout_path().await {
Ok(path) => path,
Err(err) => {
warn!("failed to resolve guardian trunk rollout path: {err}");
None
}
}
}

pub(crate) async fn shutdown(&self) {
let (review_session, ephemeral_reviews) = {
let mut state = self.state.lock().await;
Expand Down
32 changes: 32 additions & 0 deletions codex-rs/core/src/session/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4660,6 +4660,38 @@ async fn shutdown_and_wait_shuts_down_cached_guardian_subagent() {
.expect("guardian subagent should receive a shutdown op");
}

#[tokio::test]
async fn cached_guardian_subagent_exposes_its_rollout_path() {
let (parent_session, _parent_turn_context) = make_session_and_context().await;
let parent_session = Arc::new(parent_session);

let (mut child_session, _child_turn_context) = make_session_and_context().await;
let child_rollout_path = attach_thread_persistence(&mut child_session).await;
let (child_tx_sub, _child_rx_sub) = async_channel::bounded(4);
let (_child_tx_event, child_rx_event) = async_channel::unbounded();
let (_child_status_tx, child_agent_status) = watch::channel(AgentStatus::PendingInit);
let child_session_loop_handle = tokio::spawn(async {});
let child_codex = Codex {
tx_sub: child_tx_sub,
rx_event: child_rx_event,
agent_status: child_agent_status,
session: Arc::new(child_session),
session_loop_termination: session_loop_termination_from_handle(child_session_loop_handle),
};
parent_session
.guardian_review_session
.cache_for_test(child_codex)
.await;

assert_eq!(
parent_session
.guardian_review_session
.trunk_rollout_path()
.await,
Some(child_rollout_path)
);
}

#[tokio::test]
async fn shutdown_and_wait_shuts_down_tracked_ephemeral_guardian_review() {
let (parent_session, parent_turn_context) = make_session_and_context().await;
Expand Down
37 changes: 27 additions & 10 deletions codex-rs/feedback/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -338,12 +338,18 @@ pub struct FeedbackSnapshot {
pub thread_id: String,
}

pub struct FeedbackAttachmentPath {
pub path: PathBuf,
/// Optional filename to use for the uploaded attachment instead of `path`'s basename.
pub attachment_filename_override: Option<String>,
}

pub struct FeedbackUploadOptions<'a> {
pub classification: &'a str,
pub reason: Option<&'a str>,
pub tags: Option<&'a BTreeMap<String, String>>,
pub include_logs: bool,
pub extra_attachment_paths: &'a [PathBuf],
pub extra_attachment_paths: &'a [FeedbackAttachmentPath],
pub session_source: Option<SessionSource>,
pub logs_override: Option<Vec<u8>>,
}
Expand Down Expand Up @@ -501,7 +507,7 @@ impl FeedbackSnapshot {
fn feedback_attachments(
&self,
include_logs: bool,
extra_attachment_paths: &[PathBuf],
extra_attachment_paths: &[FeedbackAttachmentPath],
logs_override: Option<Vec<u8>>,
) -> Vec<sentry::protocol::Attachment> {
use sentry::protocol::Attachment;
Expand All @@ -526,22 +532,28 @@ impl FeedbackSnapshot {
});
}

for path in extra_attachment_paths {
let data = match fs::read(path) {
for attachment_path in extra_attachment_paths {
let data = match fs::read(&attachment_path.path) {
Ok(data) => data,
Err(err) => {
tracing::warn!(
path = %path.display(),
path = %attachment_path.path.display(),
error = %err,
"failed to read log attachment; skipping"
);
continue;
}
};
let filename = path
.file_name()
.map(|s| s.to_string_lossy().to_string())
.unwrap_or_else(|| "extra-log.log".to_string());
let filename = attachment_path
.attachment_filename_override
.clone()
.unwrap_or_else(|| {
attachment_path
.path
.file_name()
.map(|s| s.to_string_lossy().to_string())
.unwrap_or_else(|| "extra-log.log".to_string())
});
attachments.push(Attachment {
buffer: data,
filename,
Expand Down Expand Up @@ -676,6 +688,10 @@ mod tests {
fn feedback_attachments_gate_connectivity_diagnostics() {
let extra_filename = format!("codex-feedback-extra-{}.jsonl", ThreadId::new());
let extra_path = std::env::temp_dir().join(&extra_filename);
let extra_attachment_path = FeedbackAttachmentPath {
path: extra_path.clone(),
attachment_filename_override: None,
};
fs::write(&extra_path, "rollout").expect("extra attachment should be written");

let snapshot_with_diagnostics = CodexFeedback::new()
Expand All @@ -688,7 +704,7 @@ mod tests {

let attachments_with_diagnostics = snapshot_with_diagnostics.feedback_attachments(
/*include_logs*/ true,
std::slice::from_ref(&extra_path),
std::slice::from_ref(&extra_attachment_path),
Some(vec![1]),
);

Expand All @@ -715,6 +731,7 @@ mod tests {
);
let attachments_without_diagnostics = CodexFeedback::new()
.snapshot(/*session_id*/ None)
.with_feedback_diagnostics(FeedbackDiagnostics::default())
.feedback_attachments(/*include_logs*/ true, &[], Some(vec![1]));

assert_eq!(
Expand Down
4 changes: 4 additions & 0 deletions codex-rs/tui/src/bottom_pane/feedback_view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,7 @@ pub(crate) fn feedback_upload_consent_params(
app_event_tx: AppEventSender,
category: FeedbackCategory,
rollout_path: Option<std::path::PathBuf>,
auto_review_rollout_filename: Option<String>,
feedback_diagnostics: &FeedbackDiagnostics,
) -> super::SelectionViewParams {
use super::popup_consts::standard_popup_hint_line;
Expand Down Expand Up @@ -507,6 +508,9 @@ pub(crate) fn feedback_upload_consent_params(
{
header_lines.push(Line::from(vec![" • ".into(), name.into()]).into());
}
if let Some(filename) = auto_review_rollout_filename {
header_lines.push(Line::from(vec![" • ".into(), filename.into()]).into());
}
if !feedback_diagnostics.is_empty() {
header_lines.push(
Line::from(vec![
Expand Down
2 changes: 2 additions & 0 deletions codex-rs/tui/src/chatwidget.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2557,6 +2557,8 @@ impl ChatWidget {
self.app_event_tx.clone(),
category,
self.current_rollout_path.clone(),
self.thread_id
.map(|thread_id| format!("auto-review-rollout-{thread_id}.jsonl")),
snapshot.feedback_diagnostics(),
);
self.bottom_pane.show_selection_view(params);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ expression: popup

The following files will be sent:
• codex-logs.log
• auto-review-rollout-thread-1.jsonl
• codex-connectivity-diagnostics.txt

› 1. Yes Share the current Codex session logs with the team for
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ expression: popup

The following files will be sent:
codex-logs.log
auto-review-rollout-thread-1.jsonl
codex-connectivity-diagnostics.txt

Connectivity diagnostics
Expand Down
2 changes: 2 additions & 0 deletions codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2256,6 +2256,7 @@ async fn feedback_upload_consent_popup_snapshot() {
chat.app_event_tx.clone(),
crate::app_event::FeedbackCategory::Bug,
chat.current_rollout_path.clone(),
Some("auto-review-rollout-thread-1.jsonl".to_string()),
&codex_feedback::FeedbackDiagnostics::new(vec![codex_feedback::FeedbackDiagnostic {
headline: "Proxy environment variables are set and may affect connectivity."
.to_string(),
Expand All @@ -2275,6 +2276,7 @@ async fn feedback_good_result_consent_popup_includes_connectivity_diagnostics_fi
chat.app_event_tx.clone(),
crate::app_event::FeedbackCategory::GoodResult,
chat.current_rollout_path.clone(),
Some("auto-review-rollout-thread-1.jsonl".to_string()),
&codex_feedback::FeedbackDiagnostics::new(vec![codex_feedback::FeedbackDiagnostic {
headline: "Proxy environment variables are set and may affect connectivity."
.to_string(),
Expand Down
Loading