mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Include auto-review rollout in feedback uploads (#20064)
## Summary - include the live auto-review trunk rollout when `/feedback` uploads logs - upload that attachment as `auto-review-rollout-<parent-thread-id>.jsonl` so it is distinguishable from the parent rollout - show the same auto-review attachment name in the TUI consent popup ## Scope - this only covers the live cached auto-review trunk for the current parent thread - it does not add durable historical parent->auto-review lookup - it does not add persisted rollout support for ephemeral parallel review forks ## UI <img width="599" height="185" alt="Screenshot 2026-04-28 at 1 17 18 PM" src="https://github.com/user-attachments/assets/6a0e79c2-5d21-4702-8a89-f765778bc9e9" /> ## Validation - `cargo test -p codex-core cached_guardian_subagent_exposes_its_rollout_path` - `cargo test -p codex-feedback` - `cargo test -p codex-app-server` - `cargo test -p codex-tui feedback_upload_consent_popup_snapshot` - `cargo test -p codex-tui feedback_good_result_consent_popup_includes_connectivity_diagnostics_filename` ## Known unrelated local failures - `cargo test -p codex-core` currently fails in the pre-existing proxy env snapshot test `tools::runtimes::tests::maybe_wrap_shell_lc_with_snapshot_keeps_user_proxy_env_when_proxy_inactive` - `cargo test -p codex-tui` currently hits pre-existing `status::*` snapshot drift unrelated to this change ## Follow-Up - persist parallel auto-review fork sessions so /feedback can include their rollout history too - attach each persisted fork as its own clearly named file, for example auto-review-rollout-<parent-thread-id>-fork <n>.jsonl, instead of merging multiple Guardian sessions into one attachment - keep the same live-session-only scope initially; durable historical parent -> auto-review lookup can remain a separate decision if we later need feedback from resumed sessions
This commit is contained in:
committed by
GitHub
Unverified
parent
05fd904572
commit
5cf0adba93
@@ -294,6 +294,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;
|
||||
@@ -7849,14 +7850,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,
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -8001,6 +8021,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> {
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -4693,6 +4693,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;
|
||||
|
||||
@@ -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>>,
|
||||
}
|
||||
@@ -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;
|
||||
@@ -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,
|
||||
@@ -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()
|
||||
@@ -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]),
|
||||
);
|
||||
|
||||
@@ -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!(
|
||||
|
||||
@@ -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;
|
||||
@@ -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![
|
||||
|
||||
@@ -2587,6 +2587,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);
|
||||
|
||||
+1
@@ -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
|
||||
|
||||
+1
@@ -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
|
||||
|
||||
@@ -2440,6 +2440,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(),
|
||||
@@ -2459,6 +2460,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(),
|
||||
|
||||
Reference in New Issue
Block a user