mirror of
https://github.com/openai/codex.git
synced 2026-10-21 18:51:31 +00:00
Include TUI client logs in diagnostic uploads (#47889)
## Why Diagnostic uploads through the app server do not include the TUI client's diagnostic buffer. Attach those diagnostics when users consent to uploading logs. ## What changed - Stage the most recent 1 MiB of client logs as `client-logs.txt` on the app-server host and append it to `extra_log_files`. - Skip staging without log consent and list the optional attachment in the consent dialog. - Continue uploading the report if staging fails, noting the omitted logs in the report reason. Attempt to remove the staging directory after both successful and failed uploads. - Make app-server filesystem helpers usable by background requests. ## Testing Extend submission tests to cover log truncation, preservation of existing attachments, consent gating, staging failures, cleanup after upload failure, and error display. Update consent dialog snapshots to include the client log attachment. GitOrigin-RevId: 9790834d52772c08a147ee7c176cbbc2aa029d5f
This commit is contained in:
@@ -103,7 +103,6 @@ use codex_app_server_protocol::ConfigReadResponse;
|
||||
use codex_app_server_protocol::ConfigValueWriteParams;
|
||||
use codex_app_server_protocol::ConfigWriteResponse;
|
||||
use codex_app_server_protocol::FeedbackUploadParams;
|
||||
use codex_app_server_protocol::FeedbackUploadResponse;
|
||||
use codex_app_server_protocol::GetAccountRateLimitsResponse;
|
||||
use codex_app_server_protocol::HooksListEntry;
|
||||
use codex_app_server_protocol::ListMcpServerStatusParams;
|
||||
@@ -223,6 +222,7 @@ mod empty_state_policy;
|
||||
mod event_dispatch;
|
||||
mod exit_summary;
|
||||
mod experimental_features;
|
||||
mod feedback_upload;
|
||||
mod file_change_approvals;
|
||||
mod history_pagination;
|
||||
mod history_ui;
|
||||
|
||||
@@ -4,6 +4,7 @@
|
||||
//! limits, add-credit nudges, and feedback uploads. Results are routed back through `AppEvent` so
|
||||
//! the main event loop remains single-threaded.
|
||||
|
||||
use super::feedback_upload::fetch_feedback_upload;
|
||||
use super::plugin_mentions::fetch_plugin_mentions;
|
||||
use super::*;
|
||||
use crate::app_event::ConnectorsSnapshot;
|
||||
@@ -604,8 +605,10 @@ impl App {
|
||||
turn_id,
|
||||
include_logs,
|
||||
);
|
||||
let codex_home = app_server.codex_home_path(&self.config.codex_home);
|
||||
let feedback = self.feedback.clone();
|
||||
tokio::spawn(async move {
|
||||
let result = fetch_feedback_upload(request_handle, params)
|
||||
let result = fetch_feedback_upload(request_handle, codex_home, params, feedback)
|
||||
.await
|
||||
.map(|response| response.thread_id)
|
||||
.map_err(|err| err.to_string());
|
||||
@@ -1278,17 +1281,6 @@ pub(super) fn build_feedback_upload_params(
|
||||
}
|
||||
}
|
||||
|
||||
pub(super) async fn fetch_feedback_upload(
|
||||
request_handle: AppServerRequestHandle,
|
||||
params: FeedbackUploadParams,
|
||||
) -> Result<FeedbackUploadResponse> {
|
||||
let request_id = RequestId::String(format!("feedback-upload-{}", Uuid::new_v4()));
|
||||
request_handle
|
||||
.request_typed(ClientRequest::FeedbackUpload { request_id, params })
|
||||
.await
|
||||
.wrap_err("feedback/upload failed in TUI")
|
||||
}
|
||||
|
||||
/// Convert flat `McpServerStatus` responses into the per-server maps used by the
|
||||
/// in-process MCP subsystem (tools keyed as `mcp__{server}__{tool}`, plus
|
||||
/// per-server resource/template/auth maps). Test-only because the TUI
|
||||
|
||||
@@ -0,0 +1,76 @@
|
||||
//! Stage bounded TUI diagnostics on the app-server host for a single feedback upload.
|
||||
//! Log consent gates all staging; cleanup runs after staging or upload failures too.
|
||||
|
||||
use crate::app_server_session::fs::AppServerFileSystem;
|
||||
use codex_app_server_client::AppServerPath;
|
||||
use codex_app_server_client::AppServerRequestHandle;
|
||||
use codex_app_server_protocol::ClientRequest;
|
||||
use codex_app_server_protocol::FeedbackUploadParams;
|
||||
use codex_app_server_protocol::FeedbackUploadResponse;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_feedback::CodexFeedback;
|
||||
use color_eyre::eyre::Result;
|
||||
use color_eyre::eyre::WrapErr;
|
||||
use color_eyre::eyre::eyre;
|
||||
use std::path::PathBuf;
|
||||
use uuid::Uuid;
|
||||
|
||||
// Base64 adds only a third to this size, keeping fs/writeFile below the 16 MiB frame limit.
|
||||
const MAX_CLIENT_LOG_SIZE: usize = 1024 * 1024;
|
||||
|
||||
pub(super) async fn fetch_feedback_upload(
|
||||
request_handle: AppServerRequestHandle,
|
||||
codex_home: Option<AppServerPath>,
|
||||
mut params: FeedbackUploadParams,
|
||||
feedback: CodexFeedback,
|
||||
) -> Result<FeedbackUploadResponse> {
|
||||
let fs = AppServerFileSystem { request_handle };
|
||||
let staging_dir = codex_home.filter(|_| params.include_logs).map(|home| {
|
||||
home.join("tmp")
|
||||
.join(format!("tui-feedback-{}", Uuid::new_v4()))
|
||||
});
|
||||
if params.include_logs {
|
||||
let staged = async {
|
||||
let directory = staging_dir.as_ref().ok_or_else(|| {
|
||||
eyre!("App server did not report $CODEX_HOME; cannot stage feedback logs")
|
||||
})?;
|
||||
// These RPCs use the app-server's local filesystem, where feedback/upload
|
||||
// reads extraLogFiles, independently of the thread's executor environment.
|
||||
fs.fs_create_directory_all_path(directory).await?;
|
||||
let path = directory.join("client-logs.txt");
|
||||
let mut logs = feedback
|
||||
.snapshot(/*session_id*/ None)
|
||||
.log_attachment(/*logs_override*/ None)
|
||||
.buffer;
|
||||
logs.drain(..logs.len().saturating_sub(MAX_CLIENT_LOG_SIZE));
|
||||
fs.fs_write_file_path(&path, logs).await?;
|
||||
Ok::<_, color_eyre::Report>(path)
|
||||
}
|
||||
.await;
|
||||
match staged {
|
||||
Ok(path) => params
|
||||
.extra_log_files
|
||||
.get_or_insert_with(Vec::new)
|
||||
.push(PathBuf::from(path.as_str())),
|
||||
Err(err) => {
|
||||
tracing::warn!("failed to stage TUI feedback logs: {err}");
|
||||
params.reason.get_or_insert_with(String::new).push_str(
|
||||
"\n\nTUI client logs were omitted because they could not be staged on the app-server.",
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
let request_id = RequestId::String(format!("feedback-upload-{}", Uuid::new_v4()));
|
||||
let result = fs
|
||||
.request_handle
|
||||
.request_typed(ClientRequest::FeedbackUpload { request_id, params })
|
||||
.await
|
||||
.wrap_err("feedback/upload failed in TUI");
|
||||
|
||||
if let Some(directory) = staging_dir
|
||||
&& let Err(err) = fs.fs_remove_path(&directory).await
|
||||
{
|
||||
tracing::warn!("failed to remove staged feedback logs: {err}");
|
||||
}
|
||||
result
|
||||
}
|
||||
@@ -7185,8 +7185,90 @@ fn request_user_input_request(thread_id: ThreadId, turn_id: &str, item_id: &str)
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn feedback_submission_without_thread_emits_error_history_cell() {
|
||||
async fn feedback_submission_stages_logs_cleans_up_and_emits_error_history_cell() -> Result<()> {
|
||||
use base64::Engine;
|
||||
use base64::engine::general_purpose::STANDARD;
|
||||
use std::io::Write;
|
||||
use tracing_subscriber::fmt::writer::MakeWriter;
|
||||
|
||||
let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await;
|
||||
let (server, requests, proxy) =
|
||||
session_lifecycle_requests::start_recording_remote_app_server(&app.config).await?;
|
||||
let diagnostic = "SQLITE LOG WRITE FAILURE: disk full\n";
|
||||
let expected_logs = format!("{}{diagnostic}", "x".repeat(1024 * 1024 - diagnostic.len()));
|
||||
let mut writer = app.feedback.make_writer().make_writer();
|
||||
writer.write_all(&vec![b'x'; 1024 * 1024])?;
|
||||
writer.write_all(diagnostic.as_bytes())?;
|
||||
for (include_logs, staging_fails) in [(true, false), (false, false), (true, true)] {
|
||||
let home = tempdir()?;
|
||||
let tmp = home.path().join("tmp");
|
||||
if staging_fails {
|
||||
std::fs::write(&tmp, "not a directory")?;
|
||||
}
|
||||
let mut params = background_requests::build_feedback_upload_params(
|
||||
/*origin_thread_id*/ None,
|
||||
Some(PathBuf::from("existing-rollout.jsonl")),
|
||||
FeedbackCategory::Bug,
|
||||
/*reason*/ None,
|
||||
/*turn_id*/ None,
|
||||
include_logs,
|
||||
);
|
||||
// Reject before network upload, after the client has staged its diagnostics.
|
||||
params.thread_id = Some("invalid-thread-id".into());
|
||||
let error = feedback_upload::fetch_feedback_upload(
|
||||
server.request_handle(),
|
||||
Some(AppServerPath::from_app_server(
|
||||
home.path().display().to_string(),
|
||||
)),
|
||||
params,
|
||||
app.feedback.clone(),
|
||||
)
|
||||
.await
|
||||
.unwrap_err();
|
||||
assert!(format!("{error:#}").contains("invalid thread id"));
|
||||
let recorded = std::mem::take(&mut *requests.lock().unwrap());
|
||||
let upload = recorded
|
||||
.iter()
|
||||
.find(|r| r.method == "feedback/upload")
|
||||
.unwrap();
|
||||
let upload: FeedbackUploadParams = serde_json::from_value(upload.params.clone().unwrap())?;
|
||||
if include_logs && !staging_fails {
|
||||
let write = recorded
|
||||
.iter()
|
||||
.find(|r| r.method == "fs/writeFile")
|
||||
.unwrap();
|
||||
let write = write.params.as_ref().unwrap();
|
||||
assert_eq!(
|
||||
STANDARD.decode(write["dataBase64"].as_str().unwrap())?,
|
||||
expected_logs.as_bytes()
|
||||
);
|
||||
assert_eq!(
|
||||
upload.extra_log_files,
|
||||
Some(vec![
|
||||
PathBuf::from("existing-rollout.jsonl"),
|
||||
PathBuf::from(write["path"].as_str().unwrap())
|
||||
])
|
||||
);
|
||||
assert_eq!(std::fs::read_dir(&tmp)?.count(), 0);
|
||||
} else if staging_fails {
|
||||
assert_eq!(
|
||||
upload.extra_log_files,
|
||||
Some(vec![PathBuf::from("existing-rollout.jsonl")])
|
||||
);
|
||||
assert!(
|
||||
upload
|
||||
.reason
|
||||
.unwrap()
|
||||
.contains("TUI client logs were omitted")
|
||||
);
|
||||
} else {
|
||||
assert_eq!(recorded.len(), 1);
|
||||
assert_eq!(upload.extra_log_files, None);
|
||||
assert!(!tmp.exists());
|
||||
}
|
||||
}
|
||||
server.shutdown().await?;
|
||||
proxy.await??;
|
||||
|
||||
app.handle_feedback_submitted(
|
||||
/*origin_thread_id*/ None,
|
||||
@@ -7204,6 +7286,7 @@ async fn feedback_submission_without_thread_emits_error_history_cell() {
|
||||
lines_to_single_string(&cell.display_lines(/*width*/ 120)),
|
||||
"■ Failed to upload feedback: boom"
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -343,7 +343,7 @@ async fn cleanup_materialized_goal_files(
|
||||
output_dir: Option<goal_files::GoalFilePath>,
|
||||
) {
|
||||
if let Some(output_dir) = output_dir
|
||||
&& let Err(err) = app_server.fs_remove_path(&output_dir).await
|
||||
&& let Err(err) = app_server.file_system().fs_remove_path(&output_dir).await
|
||||
{
|
||||
tracing::warn!("failed to clean up materialized goal files at {output_dir}: {err}");
|
||||
}
|
||||
|
||||
@@ -4,7 +4,7 @@
|
||||
//! request/response plumbing out of `App` and `ChatWidget`.
|
||||
|
||||
mod external_agent_config;
|
||||
mod fs;
|
||||
pub(crate) mod fs;
|
||||
mod history;
|
||||
mod models;
|
||||
mod realtime;
|
||||
|
||||
@@ -1,3 +1,5 @@
|
||||
//! Filesystem RPCs on the app-server host, usable by sessions and background requests.
|
||||
|
||||
use super::AppServerSession;
|
||||
use base64::Engine;
|
||||
use base64::engine::general_purpose::STANDARD;
|
||||
@@ -19,12 +21,22 @@ use color_eyre::eyre::Result;
|
||||
use color_eyre::eyre::WrapErr;
|
||||
use serde::de::DeserializeOwned;
|
||||
use serde_json::json;
|
||||
use uuid::Uuid;
|
||||
|
||||
impl AppServerSession {
|
||||
pub(crate) async fn fs_create_directory_all_path(
|
||||
&mut self,
|
||||
path: &AppServerPath,
|
||||
) -> Result<()> {
|
||||
pub(crate) fn file_system(&self) -> AppServerFileSystem {
|
||||
AppServerFileSystem {
|
||||
request_handle: self.request_handle(),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) struct AppServerFileSystem {
|
||||
pub(crate) request_handle: AppServerRequestHandle,
|
||||
}
|
||||
|
||||
impl AppServerFileSystem {
|
||||
pub(crate) async fn fs_create_directory_all_path(&self, path: &AppServerPath) -> Result<()> {
|
||||
self.request_fs_path::<FsCreateDirectoryResponse>(
|
||||
"fs/createDirectory",
|
||||
path,
|
||||
@@ -42,7 +54,7 @@ impl AppServerSession {
|
||||
}
|
||||
|
||||
pub(crate) async fn fs_write_file_path(
|
||||
&mut self,
|
||||
&self,
|
||||
path: &AppServerPath,
|
||||
bytes: Vec<u8>,
|
||||
) -> Result<()> {
|
||||
@@ -63,7 +75,7 @@ impl AppServerSession {
|
||||
.map(drop)
|
||||
}
|
||||
|
||||
pub(crate) async fn fs_read_file_path(&mut self, path: &AppServerPath) -> Result<Vec<u8>> {
|
||||
pub(crate) async fn fs_read_file_path(&self, path: &AppServerPath) -> Result<Vec<u8>> {
|
||||
let response: FsReadFileResponse = self
|
||||
.request_fs_path(
|
||||
"fs/readFile",
|
||||
@@ -80,7 +92,7 @@ impl AppServerSession {
|
||||
.wrap_err("fs/readFile returned invalid base64 data")
|
||||
}
|
||||
|
||||
pub(crate) async fn fs_remove_path(&mut self, path: &AppServerPath) -> Result<()> {
|
||||
pub(crate) async fn fs_remove_path(&self, path: &AppServerPath) -> Result<()> {
|
||||
self.request_fs_path::<FsRemoveResponse>(
|
||||
"fs/remove",
|
||||
path,
|
||||
@@ -99,14 +111,14 @@ impl AppServerSession {
|
||||
}
|
||||
|
||||
async fn request_fs_path<T: DeserializeOwned>(
|
||||
&mut self,
|
||||
&self,
|
||||
method: &str,
|
||||
path: &AppServerPath,
|
||||
local_request: impl FnOnce(RequestId, AbsolutePathBuf) -> ClientRequest,
|
||||
remote_params: impl FnOnce() -> serde_json::Value,
|
||||
) -> Result<T> {
|
||||
let request_id = self.next_request_id();
|
||||
match self.request_handle() {
|
||||
let request_id = RequestId::String(format!("tui-fs-{}", Uuid::new_v4()));
|
||||
match &self.request_handle {
|
||||
AppServerRequestHandle::Remote(handle) => {
|
||||
let response = handle
|
||||
.request_json_rpc(JSONRPCRequest {
|
||||
@@ -125,7 +137,7 @@ impl AppServerSession {
|
||||
AppServerRequestHandle::InProcess(_) => {
|
||||
let path = AbsolutePathBuf::from_absolute_path_checked(path.as_str())
|
||||
.wrap_err_with(|| format!("invalid local app-server fs path {path}"))?;
|
||||
self.client
|
||||
self.request_handle
|
||||
.request_typed(local_request(request_id, path))
|
||||
.await
|
||||
.wrap_err_with(|| format!("{method} failed in TUI"))
|
||||
|
||||
@@ -253,6 +253,7 @@ pub(crate) fn feedback_upload_consent_params(
|
||||
Line::from("").into(),
|
||||
Line::from("The following files will be sent:".dim()).into(),
|
||||
Line::from(vec![" • ".into(), "codex-logs.log".into()]).into(),
|
||||
Line::from(vec![" • ".into(), "client-logs.txt (if available)".into()]).into(),
|
||||
Line::from(vec![
|
||||
" • ".into(),
|
||||
DOCTOR_REPORT_ATTACHMENT_FILENAME.into(),
|
||||
|
||||
+1
@@ -7,6 +7,7 @@ Upload logs?
|
||||
|
||||
The following files will be sent:
|
||||
• codex-logs.log
|
||||
• client-logs.txt (if available)
|
||||
• codex-doctor-report.json
|
||||
• codex-apps-tools-cache.json (if available)
|
||||
• codex-app-directory-cache.json (if available)
|
||||
|
||||
+1
@@ -7,6 +7,7 @@ Upload logs?
|
||||
|
||||
The following files will be sent:
|
||||
• codex-logs.log
|
||||
• client-logs.txt (if available)
|
||||
• codex-doctor-report.json
|
||||
• codex-apps-tools-cache.json (if available)
|
||||
• codex-app-directory-cache.json (if available)
|
||||
|
||||
+1
@@ -6,6 +6,7 @@ expression: popup
|
||||
|
||||
The following files will be sent:
|
||||
• codex-logs.log
|
||||
• client-logs.txt (if available)
|
||||
• codex-doctor-report.json
|
||||
• codex-apps-tools-cache.json (if available)
|
||||
• codex-app-directory-cache.json (if available)
|
||||
|
||||
+1
@@ -6,6 +6,7 @@ expression: popup
|
||||
|
||||
The following files will be sent:
|
||||
• codex-logs.log
|
||||
• client-logs.txt (if available)
|
||||
• codex-doctor-report.json
|
||||
• codex-apps-tools-cache.json (if available)
|
||||
• codex-app-directory-cache.json (if available)
|
||||
|
||||
@@ -126,7 +126,7 @@ pub(crate) async fn materialize_goal_draft(
|
||||
Ok(reference) => reference,
|
||||
Err(err) => {
|
||||
if let Some(output_dir) = output_dir.as_ref() {
|
||||
let _ = app_server.fs_remove_path(output_dir).await;
|
||||
let _ = app_server.file_system().fs_remove_path(output_dir).await;
|
||||
}
|
||||
return Err(err);
|
||||
}
|
||||
@@ -146,6 +146,7 @@ pub(crate) async fn objective_text_for_edit(
|
||||
return Ok(objective.to_string());
|
||||
};
|
||||
let bytes = app_server
|
||||
.file_system()
|
||||
.fs_read_file_path(&path)
|
||||
.await
|
||||
.map_err(|err| anyhow::anyhow!("{err}"))
|
||||
@@ -196,6 +197,7 @@ async fn ensure_goal_output_dir(
|
||||
.join(GOAL_ATTACHMENT_DIR)
|
||||
.join(Uuid::new_v4().to_string());
|
||||
app_server
|
||||
.file_system()
|
||||
.fs_create_directory_all_path(&path)
|
||||
.await
|
||||
.map_err(|err| anyhow::anyhow!("{err}"))
|
||||
@@ -210,6 +212,7 @@ async fn write_goal_file(
|
||||
bytes: Vec<u8>,
|
||||
) -> Result<()> {
|
||||
app_server
|
||||
.file_system()
|
||||
.fs_write_file_path(&path, bytes)
|
||||
.await
|
||||
.map_err(|err| anyhow::anyhow!("{err}"))
|
||||
|
||||
Reference in New Issue
Block a user