mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Pass turn id with feedback uploads (#17314)
## Summary - Add an optional `tags` dictionary to feedback upload params. - Capture the active app-server turn id in the TUI and submit it as `tags.turn_id` with `/feedback` uploads. - Merge client-provided feedback tags into Sentry feedback tags while preserving reserved system fields like `thread_id`, `classification`, `cli_version`, `session_source`, and `reason`. ## Behavior / impact Existing feedback upload callers remain compatible because `tags` is optional and nullable. The wire shape is still a normal JSON object / TypeScript dictionary, so adding future feedback metadata will not require a new top-level protocol field each time. This change only adds feedback metadata for Codex CLI/TUI uploads; it does not affect existing pipelines, DAGs, exports, or downstream consumers unless they choose to read the new `turn_id` feedback tag. ## Tests - `cargo fmt -- --config imports_granularity=Item` passed; stable rustfmt warned that `imports_granularity` is nightly-only. - `cargo run -p codex-app-server-protocol --bin write_schema_fixtures` - `cargo test -p codex-feedback upload_tags_include_client_tags_and_preserve_reserved_fields` - `cargo test -p codex-app-server-protocol schema_fixtures_match_generated` - `cargo test -p codex-tui build_feedback_upload_params` - `cargo test -p codex-tui live_app_server_turn_started_sets_feedback_turn_id` - `cargo check -p codex-app-server --tests` - `git diff --check` --------- Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
+18
-1
@@ -2020,6 +2020,7 @@ impl App {
|
||||
app_server: &AppServerSession,
|
||||
category: FeedbackCategory,
|
||||
reason: Option<String>,
|
||||
turn_id: Option<String>,
|
||||
include_logs: bool,
|
||||
) {
|
||||
let request_handle = app_server.request_handle();
|
||||
@@ -2035,6 +2036,7 @@ impl App {
|
||||
rollout_path,
|
||||
category,
|
||||
reason,
|
||||
turn_id,
|
||||
include_logs,
|
||||
);
|
||||
tokio::spawn(async move {
|
||||
@@ -4630,9 +4632,10 @@ impl App {
|
||||
AppEvent::SubmitFeedback {
|
||||
category,
|
||||
reason,
|
||||
turn_id,
|
||||
include_logs,
|
||||
} => {
|
||||
self.submit_feedback(app_server, category, reason, include_logs);
|
||||
self.submit_feedback(app_server, category, reason, turn_id, include_logs);
|
||||
}
|
||||
AppEvent::FeedbackSubmitted {
|
||||
origin_thread_id,
|
||||
@@ -6249,6 +6252,7 @@ fn build_feedback_upload_params(
|
||||
rollout_path: Option<PathBuf>,
|
||||
category: FeedbackCategory,
|
||||
reason: Option<String>,
|
||||
turn_id: Option<String>,
|
||||
include_logs: bool,
|
||||
) -> FeedbackUploadParams {
|
||||
let extra_log_files = if include_logs {
|
||||
@@ -6256,12 +6260,14 @@ fn build_feedback_upload_params(
|
||||
} else {
|
||||
None
|
||||
};
|
||||
let tags = turn_id.map(|turn_id| BTreeMap::from([(String::from("turn_id"), turn_id)]));
|
||||
FeedbackUploadParams {
|
||||
classification: crate::bottom_pane::feedback_classification(category).to_string(),
|
||||
reason,
|
||||
thread_id: origin_thread_id.map(|thread_id| thread_id.to_string()),
|
||||
include_logs,
|
||||
extra_log_files,
|
||||
tags,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -9644,12 +9650,21 @@ guardian_approval = true
|
||||
Some(rollout_path.clone()),
|
||||
FeedbackCategory::SafetyCheck,
|
||||
Some("needs follow-up".to_string()),
|
||||
Some("turn-123".to_string()),
|
||||
/*include_logs*/ true,
|
||||
);
|
||||
|
||||
assert_eq!(params.classification, "safety_check");
|
||||
assert_eq!(params.reason, Some("needs follow-up".to_string()));
|
||||
assert_eq!(params.thread_id, Some(thread_id.to_string()));
|
||||
assert_eq!(
|
||||
params
|
||||
.tags
|
||||
.as_ref()
|
||||
.and_then(|tags| tags.get("turn_id"))
|
||||
.map(String::as_str),
|
||||
Some("turn-123")
|
||||
);
|
||||
assert_eq!(params.include_logs, true);
|
||||
assert_eq!(params.extra_log_files, Some(vec![rollout_path]));
|
||||
}
|
||||
@@ -9661,12 +9676,14 @@ guardian_approval = true
|
||||
Some(PathBuf::from("/tmp/rollout.jsonl")),
|
||||
FeedbackCategory::GoodResult,
|
||||
/*reason*/ None,
|
||||
/*turn_id*/ None,
|
||||
/*include_logs*/ false,
|
||||
);
|
||||
|
||||
assert_eq!(params.classification, "good_result");
|
||||
assert_eq!(params.reason, None);
|
||||
assert_eq!(params.thread_id, None);
|
||||
assert_eq!(params.tags, None);
|
||||
assert_eq!(params.include_logs, false);
|
||||
assert_eq!(params.extra_log_files, None);
|
||||
}
|
||||
|
||||
@@ -559,6 +559,7 @@ pub(crate) enum AppEvent {
|
||||
SubmitFeedback {
|
||||
category: FeedbackCategory,
|
||||
reason: Option<String>,
|
||||
turn_id: Option<String>,
|
||||
include_logs: bool,
|
||||
},
|
||||
|
||||
|
||||
@@ -45,6 +45,7 @@ pub(crate) enum FeedbackAudience {
|
||||
/// through the app-server-managed feedback flow.
|
||||
pub(crate) struct FeedbackNoteView {
|
||||
category: FeedbackCategory,
|
||||
turn_id: Option<String>,
|
||||
app_event_tx: AppEventSender,
|
||||
include_logs: bool,
|
||||
|
||||
@@ -57,11 +58,13 @@ pub(crate) struct FeedbackNoteView {
|
||||
impl FeedbackNoteView {
|
||||
pub(crate) fn new(
|
||||
category: FeedbackCategory,
|
||||
turn_id: Option<String>,
|
||||
app_event_tx: AppEventSender,
|
||||
include_logs: bool,
|
||||
) -> Self {
|
||||
Self {
|
||||
category,
|
||||
turn_id,
|
||||
app_event_tx,
|
||||
include_logs,
|
||||
textarea: TextArea::new(),
|
||||
@@ -76,6 +79,7 @@ impl FeedbackNoteView {
|
||||
self.app_event_tx.send(AppEvent::SubmitFeedback {
|
||||
category: self.category,
|
||||
reason,
|
||||
turn_id: self.turn_id.clone(),
|
||||
include_logs: self.include_logs,
|
||||
});
|
||||
self.complete = true;
|
||||
@@ -607,7 +611,9 @@ mod tests {
|
||||
fn make_view(category: FeedbackCategory) -> FeedbackNoteView {
|
||||
let (tx_raw, _rx) = tokio::sync::mpsc::unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx_raw);
|
||||
FeedbackNoteView::new(category, tx, /*include_logs*/ true)
|
||||
FeedbackNoteView::new(
|
||||
category, /*turn_id*/ None, tx, /*include_logs*/ true,
|
||||
)
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -649,7 +655,12 @@ mod tests {
|
||||
fn feedback_view_with_connectivity_diagnostics() {
|
||||
let (tx_raw, _rx) = tokio::sync::mpsc::unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx_raw);
|
||||
let view = FeedbackNoteView::new(FeedbackCategory::Bug, tx, /*include_logs*/ false);
|
||||
let view = FeedbackNoteView::new(
|
||||
FeedbackCategory::Bug,
|
||||
/*turn_id*/ None,
|
||||
tx,
|
||||
/*include_logs*/ false,
|
||||
);
|
||||
let rendered = render(&view, /*width*/ 60);
|
||||
|
||||
insta::assert_snapshot!("feedback_view_with_connectivity_diagnostics", rendered);
|
||||
@@ -659,7 +670,12 @@ mod tests {
|
||||
fn submit_feedback_emits_submit_event_with_trimmed_note() {
|
||||
let (tx_raw, mut rx) = tokio::sync::mpsc::unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx_raw);
|
||||
let mut view = FeedbackNoteView::new(FeedbackCategory::Bug, tx, /*include_logs*/ true);
|
||||
let mut view = FeedbackNoteView::new(
|
||||
FeedbackCategory::Bug,
|
||||
Some("turn-123".to_string()),
|
||||
tx,
|
||||
/*include_logs*/ true,
|
||||
);
|
||||
view.textarea.insert_str(" something broke ");
|
||||
|
||||
view.submit();
|
||||
@@ -670,8 +686,9 @@ mod tests {
|
||||
AppEvent::SubmitFeedback {
|
||||
category: FeedbackCategory::Bug,
|
||||
reason: Some(reason),
|
||||
turn_id: Some(turn_id),
|
||||
include_logs: true,
|
||||
} if reason == "something broke"
|
||||
} if reason == "something broke" && turn_id == "turn-123"
|
||||
));
|
||||
assert_eq!(view.is_complete(), true);
|
||||
}
|
||||
@@ -682,6 +699,7 @@ mod tests {
|
||||
let tx = AppEventSender::new(tx_raw);
|
||||
let mut view = FeedbackNoteView::new(
|
||||
FeedbackCategory::GoodResult,
|
||||
/*turn_id*/ None,
|
||||
tx,
|
||||
/*include_logs*/ false,
|
||||
);
|
||||
@@ -694,6 +712,7 @@ mod tests {
|
||||
AppEvent::SubmitFeedback {
|
||||
category: FeedbackCategory::GoodResult,
|
||||
reason: None,
|
||||
turn_id: None,
|
||||
include_logs: false,
|
||||
}
|
||||
));
|
||||
|
||||
@@ -854,6 +854,7 @@ pub(crate) struct ChatWidget {
|
||||
pending_status_indicator_restore: bool,
|
||||
suppress_queue_autosend: bool,
|
||||
thread_id: Option<ThreadId>,
|
||||
last_turn_id: Option<String>,
|
||||
thread_name: Option<String>,
|
||||
forked_from: Option<ThreadId>,
|
||||
frame_requester: FrameRequester,
|
||||
@@ -1961,6 +1962,7 @@ impl ChatWidget {
|
||||
self.set_skills(/*skills*/ None);
|
||||
self.session_network_proxy = event.network_proxy.clone();
|
||||
self.thread_id = Some(event.session_id);
|
||||
self.last_turn_id = None;
|
||||
self.thread_name = event.thread_name.clone();
|
||||
self.forked_from = event.forked_from_id;
|
||||
self.current_rollout_path = event.rollout_path.clone();
|
||||
@@ -2143,6 +2145,7 @@ impl ChatWidget {
|
||||
) {
|
||||
let view = crate::bottom_pane::FeedbackNoteView::new(
|
||||
category,
|
||||
self.last_turn_id.clone(),
|
||||
self.app_event_tx.clone(),
|
||||
include_logs,
|
||||
);
|
||||
@@ -4827,6 +4830,7 @@ impl ChatWidget {
|
||||
pending_status_indicator_restore: false,
|
||||
suppress_queue_autosend: false,
|
||||
thread_id: None,
|
||||
last_turn_id: None,
|
||||
thread_name: None,
|
||||
forked_from: None,
|
||||
queued_user_messages: VecDeque::new(),
|
||||
@@ -6501,7 +6505,8 @@ impl ChatWidget {
|
||||
}
|
||||
}
|
||||
}
|
||||
ServerNotification::TurnStarted(_) => {
|
||||
ServerNotification::TurnStarted(notification) => {
|
||||
self.last_turn_id = Some(notification.turn.id);
|
||||
self.last_non_retry_error = None;
|
||||
if !matches!(replay_kind, Some(ReplayKind::ResumeInitialMessages)) {
|
||||
self.on_task_started();
|
||||
@@ -7072,8 +7077,11 @@ impl ChatWidget {
|
||||
}
|
||||
EventMsg::AgentReasoningSectionBreak(_) => self.on_reasoning_section_break(),
|
||||
EventMsg::TurnStarted(event) => {
|
||||
let turn_id = event.turn_id;
|
||||
let model_context_window = event.model_context_window;
|
||||
self.last_turn_id = Some(turn_id);
|
||||
if !is_resume_initial_replay {
|
||||
self.apply_turn_started_context_window(event.model_context_window);
|
||||
self.apply_turn_started_context_window(model_context_window);
|
||||
self.on_task_started();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -147,6 +147,43 @@ async fn live_app_server_turn_completed_clears_working_status_after_answer_item(
|
||||
assert!(chat.bottom_pane.status_widget().is_none());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn live_app_server_turn_started_sets_feedback_turn_id() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
|
||||
chat.handle_server_notification(
|
||||
ServerNotification::TurnStarted(TurnStartedNotification {
|
||||
thread_id: "thread-1".to_string(),
|
||||
turn: AppServerTurn {
|
||||
id: "turn-1".to_string(),
|
||||
items: Vec::new(),
|
||||
status: AppServerTurnStatus::InProgress,
|
||||
error: None,
|
||||
started_at: Some(0),
|
||||
completed_at: None,
|
||||
duration_ms: None,
|
||||
},
|
||||
}),
|
||||
/*replay_kind*/ None,
|
||||
);
|
||||
|
||||
chat.open_feedback_note(
|
||||
crate::app_event::FeedbackCategory::Bug,
|
||||
/*include_logs*/ false,
|
||||
);
|
||||
chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
|
||||
|
||||
assert_matches!(
|
||||
rx.try_recv(),
|
||||
Ok(AppEvent::SubmitFeedback {
|
||||
category: crate::app_event::FeedbackCategory::Bug,
|
||||
reason: None,
|
||||
turn_id: Some(turn_id),
|
||||
include_logs: false,
|
||||
}) if turn_id == "turn-1"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn live_app_server_file_change_item_started_preserves_changes() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
|
||||
@@ -244,6 +244,7 @@ pub(super) async fn make_chatwidget_manual(
|
||||
pending_status_indicator_restore: false,
|
||||
suppress_queue_autosend: false,
|
||||
thread_id: None,
|
||||
last_turn_id: None,
|
||||
thread_name: None,
|
||||
forked_from: None,
|
||||
frame_requester: FrameRequester::test_dummy(),
|
||||
|
||||
Reference in New Issue
Block a user