mirror of
https://github.com/openai/codex.git
synced 2026-10-22 17:58:46 +00:00
Add local diagnostic ZIP exports to the app server (#55940)
## What changed Add a save RPC that accepts the existing report options and an absolute `savePath` on the server host. Save the report, selected attachments, and a manifest without uploading, and return `savedPath`, `threadId`, and `promptHash`. Local exports remain available when uploads are disabled by configuration. Publish archives atomically without replacing existing files. Bound uncompressed contents to 200 MiB and attachments to 128, preserve duplicate attachment names safely, and fail without publishing a partial ZIP when a selected file cannot be read. Honor `includeLogs` and omit automatic rollouts for untouched threads without materializing them. Update protocol schemas and generated TypeScript and Python types. ## Testing Add coverage for local-only saves, disabled uploads, path validation, overwrite protection, log consent, archive budgets, duplicate names, missing files, and rollout selection before materialization and after writer shutdown. GitOrigin-RevId: 222c0fe7ab923904de9c8a9f12ad27f7a0104875
This commit is contained in:
viktor-oai
@@ -26,6 +26,7 @@ tempfile = { workspace = true }
|
||||
tokio = { workspace = true, features = ["time"] }
|
||||
tracing = { workspace = true }
|
||||
tracing-subscriber = { workspace = true }
|
||||
zip = { workspace = true }
|
||||
|
||||
[dev-dependencies]
|
||||
log = { workspace = true }
|
||||
|
||||
@@ -0,0 +1,127 @@
|
||||
//! Atomic local feedback export with bounded contents and no file replacement.
|
||||
|
||||
use std::io;
|
||||
use std::io::Write;
|
||||
use std::path::Path;
|
||||
|
||||
use crate::FeedbackAttachment;
|
||||
use crate::FeedbackAttachmentPath;
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
|
||||
pub(crate) enum Input<'a> {
|
||||
Owned(FeedbackAttachment),
|
||||
Path(&'a FeedbackAttachmentPath),
|
||||
}
|
||||
use zip::ZipWriter;
|
||||
use zip::write::SimpleFileOptions;
|
||||
|
||||
pub(crate) struct Limits {
|
||||
pub max_bytes: usize,
|
||||
pub max_attachments: usize,
|
||||
}
|
||||
|
||||
impl Default for Limits {
|
||||
fn default() -> Self {
|
||||
Self {
|
||||
max_bytes: 200 * 1024 * 1024,
|
||||
max_attachments: 128,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Count uncompressed entry contents, including report and manifest serialization.
|
||||
struct BudgetWriter<'a, W> {
|
||||
inner: &'a mut W,
|
||||
remaining: &'a mut usize,
|
||||
}
|
||||
|
||||
impl<W: Write> Write for BudgetWriter<'_, W> {
|
||||
fn write(&mut self, buffer: &[u8]) -> io::Result<usize> {
|
||||
let remaining = self.remaining.checked_sub(buffer.len()).ok_or_else(|| {
|
||||
io::Error::other("feedback archive exceeds the uncompressed size limit")
|
||||
})?;
|
||||
self.inner.write_all(buffer)?;
|
||||
*self.remaining = remaining;
|
||||
Ok(buffer.len())
|
||||
}
|
||||
|
||||
fn flush(&mut self) -> io::Result<()> {
|
||||
self.inner.flush()
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn save<'a>(
|
||||
path: &Path,
|
||||
report: &[u8],
|
||||
attachments: impl Iterator<Item = Input<'a>>,
|
||||
limits: Limits,
|
||||
) -> Result<()> {
|
||||
anyhow::ensure!(path.is_absolute(), "feedback save path must be absolute");
|
||||
anyhow::ensure!(!path.exists(), "feedback save path already exists");
|
||||
let parent = path.parent().context("feedback save path has no parent")?;
|
||||
let mut temporary = tempfile::NamedTempFile::new_in(parent)?;
|
||||
let mut writer = ZipWriter::new(temporary.as_file_mut());
|
||||
let options = SimpleFileOptions::default()
|
||||
.compression_method(zip::CompressionMethod::Deflated)
|
||||
.large_file(true);
|
||||
let mut remaining = limits.max_bytes;
|
||||
writer.start_file("report.json", options)?;
|
||||
BudgetWriter {
|
||||
inner: &mut writer,
|
||||
remaining: &mut remaining,
|
||||
}
|
||||
.write_all(report)?;
|
||||
let mut manifest = Vec::new();
|
||||
for (index, attachment) in attachments.enumerate() {
|
||||
// Inspect metadata before reading the next file or compressing its contents.
|
||||
anyhow::ensure!(
|
||||
index < limits.max_attachments,
|
||||
"feedback archive exceeds the attachment count limit"
|
||||
);
|
||||
let attachment = match attachment {
|
||||
Input::Owned(attachment) => attachment,
|
||||
Input::Path(path) => path.read_attachment(remaining)
|
||||
.with_context(|| format!("failed to read selected attachment {}", path.path.display()))?
|
||||
.with_context(|| format!("selected attachment {} is missing, nonregular, or exceeds the remaining decoded size limit", path.path.display()))?,
|
||||
};
|
||||
let basename = attachment
|
||||
.filename
|
||||
.rsplit(['/', '\\'])
|
||||
.next()
|
||||
.filter(|name| !name.is_empty() && *name != "." && *name != "..")
|
||||
.unwrap_or("attachment");
|
||||
// Numbered directories retain duplicate names without unsafe ZIP paths.
|
||||
let entry = format!("attachments/{index:04}/{basename}");
|
||||
writer.start_file(&entry, options)?;
|
||||
BudgetWriter {
|
||||
inner: &mut writer,
|
||||
remaining: &mut remaining,
|
||||
}
|
||||
.write_all(&attachment.buffer)?;
|
||||
manifest.push(serde_json::json!({
|
||||
"entry": entry,
|
||||
"filename": attachment.filename,
|
||||
"contentType": attachment.content_type,
|
||||
}));
|
||||
}
|
||||
writer.start_file("manifest.json", options)?;
|
||||
serde_json::to_writer(
|
||||
BudgetWriter {
|
||||
inner: &mut writer,
|
||||
remaining: &mut remaining,
|
||||
},
|
||||
&manifest,
|
||||
)?;
|
||||
writer.finish()?;
|
||||
temporary.as_file().sync_all()?;
|
||||
// Same-directory publication is atomic and must not replace an existing file.
|
||||
temporary
|
||||
.persist_noclobber(path)
|
||||
.map_err(|error| error.error)?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "archive_tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,175 @@
|
||||
use super::*;
|
||||
use crate::CodexFeedback;
|
||||
use crate::FeedbackUploadOptions;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::fs::File;
|
||||
use std::io::Read;
|
||||
use zip::ZipArchive;
|
||||
|
||||
fn options() -> FeedbackUploadOptions<'static> {
|
||||
FeedbackUploadOptions {
|
||||
classification: "bug",
|
||||
reason: Some("synthetic feedback"),
|
||||
tags: None,
|
||||
include_logs: false,
|
||||
extra_attachments: Vec::new(),
|
||||
extra_attachment_paths: &[],
|
||||
session_source: None,
|
||||
logs_override: Some(b"consented log bytes".to_vec()),
|
||||
}
|
||||
}
|
||||
|
||||
/// Exports preserve attachment bytes and duplicate names without unsafe ZIP paths.
|
||||
#[test]
|
||||
fn archive_preserves_selected_bytes_and_safe_names() -> Result<()> {
|
||||
let directory = tempfile::tempdir()?;
|
||||
let path = directory.path().join("feedback.zip");
|
||||
let expected = [
|
||||
("../../same.log", b"first".as_slice()),
|
||||
("../../same.log", b"second"),
|
||||
("rollouts.tar.gz", b"compressed rollout bytes"),
|
||||
];
|
||||
super::save(
|
||||
&path,
|
||||
b"{}",
|
||||
expected.iter().map(|(filename, buffer)| {
|
||||
Input::Owned(FeedbackAttachment {
|
||||
filename: filename.to_string(),
|
||||
buffer: buffer.to_vec(),
|
||||
content_type: None,
|
||||
})
|
||||
}),
|
||||
Limits::default(),
|
||||
)?;
|
||||
let mut archive = ZipArchive::new(File::open(&path)?)?;
|
||||
assert_eq!(archive.len(), 5);
|
||||
let manifest: serde_json::Value = serde_json::from_reader(archive.by_name("manifest.json")?)?;
|
||||
for (index, (filename, bytes)) in expected.iter().enumerate() {
|
||||
let basename = filename.rsplit('/').next().unwrap();
|
||||
let mut entry = archive.by_name(&format!("attachments/{index:04}/{basename}"))?;
|
||||
let mut actual = Vec::new();
|
||||
entry.read_to_end(&mut actual)?;
|
||||
assert_eq!(actual, *bytes);
|
||||
assert_eq!(manifest[index]["filename"], *filename);
|
||||
assert_eq!(entry.compression(), zip::CompressionMethod::Deflated);
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Byte and count budgets include metadata and reject excess input without leaving a partial ZIP.
|
||||
#[test]
|
||||
fn archive_bounds_all_contents_and_cleans_failed_exports() -> Result<()> {
|
||||
let directory = tempfile::tempdir()?;
|
||||
let path = directory.path().join("feedback.zip");
|
||||
// Filtering removes the exact size hint, exercising streaming attachment limits.
|
||||
let attachments = |count| {
|
||||
(0..count).filter(|_| true).map(|_| {
|
||||
Input::Owned(FeedbackAttachment {
|
||||
filename: "test.log".to_string(),
|
||||
buffer: b"synthetic".to_vec(),
|
||||
content_type: None,
|
||||
})
|
||||
})
|
||||
};
|
||||
super::save(
|
||||
&path,
|
||||
b"{}",
|
||||
attachments(2),
|
||||
Limits {
|
||||
max_bytes: 1024,
|
||||
max_attachments: 2,
|
||||
},
|
||||
)?;
|
||||
let mut archive = ZipArchive::new(File::open(&path)?)?;
|
||||
let bytes = (0..archive.len()).try_fold(0, |sum, index| {
|
||||
Ok::<_, zip::result::ZipError>(sum + archive.by_index(index)?.size() as usize)
|
||||
})?;
|
||||
drop(archive);
|
||||
std::fs::remove_file(&path)?;
|
||||
super::save(
|
||||
&path,
|
||||
b"{}",
|
||||
attachments(2),
|
||||
Limits {
|
||||
max_bytes: bytes,
|
||||
max_attachments: 2,
|
||||
},
|
||||
)?;
|
||||
std::fs::remove_file(&path)?;
|
||||
for (count, max_bytes, error) in [
|
||||
(2, bytes - 1, "uncompressed size limit"),
|
||||
(0, 1, "uncompressed size limit"),
|
||||
(3, 1024, "attachment count limit"),
|
||||
] {
|
||||
let failure = super::save(
|
||||
&path,
|
||||
b"{}",
|
||||
attachments(count),
|
||||
Limits {
|
||||
max_bytes,
|
||||
max_attachments: 2,
|
||||
},
|
||||
)
|
||||
.unwrap_err();
|
||||
assert!(failure.to_string().contains(error), "{failure:#}");
|
||||
assert_eq!(std::fs::read_dir(directory.path())?.count(), 0);
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// A local report keeps its description and includes log bytes only with consent.
|
||||
#[test]
|
||||
fn save_preserves_report_and_log_consent() -> Result<()> {
|
||||
let directory = tempfile::tempdir()?;
|
||||
for include_logs in [false, true] {
|
||||
let path = directory.path().join(format!("{include_logs}.zip"));
|
||||
CodexFeedback::new()
|
||||
.snapshot(/*session_id*/ None)
|
||||
.save_feedback(
|
||||
FeedbackUploadOptions {
|
||||
include_logs,
|
||||
..options()
|
||||
},
|
||||
&path,
|
||||
)?;
|
||||
let mut archive = ZipArchive::new(File::open(path)?)?;
|
||||
let report: serde_json::Value = serde_json::from_reader(archive.by_name("report.json")?)?;
|
||||
assert_eq!(
|
||||
report["exception"]["values"][0]["value"],
|
||||
"synthetic feedback"
|
||||
);
|
||||
let logs = archive
|
||||
.file_names()
|
||||
.collect::<zip::result::ZipResult<Vec<_>>>()?
|
||||
.into_iter()
|
||||
.find(|name| name.ends_with("/codex-logs.log"))
|
||||
.map(std::borrow::Cow::into_owned);
|
||||
assert_eq!(logs.is_some(), include_logs);
|
||||
if let Some(logs) = logs {
|
||||
let mut bytes = Vec::new();
|
||||
archive.by_name(&logs)?.read_to_end(&mut bytes)?;
|
||||
assert_eq!(bytes, b"consented log bytes");
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Oversized reports fail before publishing, using the serialized report size.
|
||||
#[test]
|
||||
fn save_rejects_oversized_events_without_publishing() -> Result<()> {
|
||||
let directory = tempfile::tempdir()?;
|
||||
let reason = "x".repeat(crate::MAX_EVENT_BYTES);
|
||||
let error = CodexFeedback::new()
|
||||
.snapshot(/*session_id*/ None)
|
||||
.save_feedback(
|
||||
FeedbackUploadOptions {
|
||||
reason: Some(&reason),
|
||||
..options()
|
||||
},
|
||||
&directory.path().join("oversized.zip"),
|
||||
)
|
||||
.unwrap_err();
|
||||
assert_eq!(error.to_string(), "feedback event exceeds the size limit");
|
||||
assert_eq!(std::fs::read_dir(directory.path())?.count(), 0);
|
||||
Ok(())
|
||||
}
|
||||
@@ -31,6 +31,7 @@ use tracing_subscriber::filter::Targets;
|
||||
use tracing_subscriber::fmt::writer::MakeWriter;
|
||||
use tracing_subscriber::registry::LookupSpan;
|
||||
|
||||
mod archive;
|
||||
mod daemon_logs;
|
||||
pub use daemon_logs::daemon_log_attachments;
|
||||
pub(crate) mod feedback_diagnostics;
|
||||
@@ -652,6 +653,46 @@ impl FeedbackSnapshot {
|
||||
.await
|
||||
}
|
||||
|
||||
/// Atomically saves the consent-selected report and attachments without uploading.
|
||||
/// Existing paths are rejected and failures never publish a partial archive.
|
||||
pub fn save_feedback(&self, options: FeedbackUploadOptions<'_>, path: &Path) -> Result<()> {
|
||||
let report = serde_json::to_vec(&self.feedback_event(
|
||||
options.classification,
|
||||
options.reason,
|
||||
options.tags,
|
||||
options.session_source.as_ref(),
|
||||
))?;
|
||||
anyhow::ensure!(
|
||||
report.len() <= MAX_EVENT_BYTES,
|
||||
"feedback event exceeds the size limit"
|
||||
);
|
||||
let logs = options
|
||||
.include_logs
|
||||
.then(|| self.log_attachment(options.logs_override));
|
||||
let diagnostics = self
|
||||
.feedback_diagnostics_attachment_text(options.include_logs)
|
||||
.map(|text| FeedbackAttachment {
|
||||
buffer: text.into_bytes(),
|
||||
filename: FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME.to_string(),
|
||||
content_type: Some("text/plain".to_string()),
|
||||
});
|
||||
archive::save(
|
||||
path,
|
||||
&report,
|
||||
logs.into_iter()
|
||||
.chain(options.extra_attachments)
|
||||
.chain(diagnostics)
|
||||
.map(archive::Input::Owned)
|
||||
.chain(
|
||||
options
|
||||
.extra_attachment_paths
|
||||
.iter()
|
||||
.map(archive::Input::Path),
|
||||
),
|
||||
archive::Limits::default(),
|
||||
)
|
||||
}
|
||||
|
||||
async fn upload_feedback_with_dsn(
|
||||
&self,
|
||||
options: FeedbackUploadOptions<'_>,
|
||||
@@ -1863,3 +1904,6 @@ mod tests {
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod save_rollout_tests;
|
||||
|
||||
@@ -0,0 +1,107 @@
|
||||
//! Regression coverage for complete, bounded file selection during local saving.
|
||||
use super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
fn rollout_name() -> String {
|
||||
format!("rollout-2026-10-19T12-00-00-{}.jsonl", ThreadId::new())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn save_bounds_files_before_reading_or_compression() -> Result<()> {
|
||||
let inputs = tempfile::tempdir()?;
|
||||
let output = tempfile::tempdir()?;
|
||||
let path = inputs.path().join(rollout_name());
|
||||
fs::write(&path, vec![b' '; 1024])?;
|
||||
let path = FeedbackAttachmentPath {
|
||||
path,
|
||||
attachment_filename_override: None,
|
||||
};
|
||||
let missing = FeedbackAttachmentPath {
|
||||
path: inputs.path().join("missing"),
|
||||
attachment_filename_override: None,
|
||||
};
|
||||
for (max_bytes, max_attachments, second, expected_error) in [
|
||||
(1500, 2, &path, "decoded size limit"),
|
||||
(4096, 1, &missing, "count limit"),
|
||||
] {
|
||||
let error = archive::save(
|
||||
&output.path().join("feedback.zip"),
|
||||
b"{}",
|
||||
[
|
||||
archive::Input::Owned(FeedbackAttachment {
|
||||
filename: rollout_name(),
|
||||
content_type: None,
|
||||
buffer: vec![b' '; 1024],
|
||||
}),
|
||||
archive::Input::Path(second),
|
||||
]
|
||||
.into_iter(),
|
||||
archive::Limits {
|
||||
max_bytes,
|
||||
max_attachments,
|
||||
},
|
||||
)
|
||||
.unwrap_err();
|
||||
assert!(format!("{error:#}").contains(expected_error), "{error:#}");
|
||||
assert_eq!(fs::read_dir(output.path())?.count(), 0);
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn save_preserves_duplicate_names_and_rejects_missing_selected_files() -> Result<()> {
|
||||
let inputs = tempfile::tempdir()?;
|
||||
let output = tempfile::tempdir()?;
|
||||
let name = rollout_name();
|
||||
let paths = [
|
||||
inputs.path().join(&name),
|
||||
inputs.path().join("nested").join(&name),
|
||||
];
|
||||
fs::create_dir(paths[1].parent().unwrap())?;
|
||||
fs::write(&paths[0], b"first\n")?;
|
||||
fs::write(&paths[1], b"second\n")?;
|
||||
let paths = paths.map(|path| FeedbackAttachmentPath {
|
||||
path,
|
||||
attachment_filename_override: None,
|
||||
});
|
||||
let destination = output.path().join("feedback.zip");
|
||||
archive::save(
|
||||
&destination,
|
||||
b"{}",
|
||||
paths.iter().map(archive::Input::Path),
|
||||
archive::Limits::default(),
|
||||
)?;
|
||||
let mut zip = zip::ZipArchive::new(fs::File::open(&destination)?)?;
|
||||
for (index, expected) in [b"first\n".as_slice(), b"second\n"].iter().enumerate() {
|
||||
let mut actual = Vec::new();
|
||||
zip.by_name(&format!("attachments/{index:04}/{name}"))?
|
||||
.read_to_end(&mut actual)?;
|
||||
assert_eq!(&actual, expected);
|
||||
}
|
||||
drop(zip);
|
||||
fs::remove_file(&destination)?;
|
||||
fs::remove_file(&paths[1].path)?;
|
||||
let error = archive::save(
|
||||
&destination,
|
||||
b"{}",
|
||||
paths.iter().map(archive::Input::Path),
|
||||
archive::Limits::default(),
|
||||
)
|
||||
.unwrap_err();
|
||||
assert!(format!("{error:#}").contains("selected attachment"));
|
||||
assert_eq!(fs::read_dir(output.path())?.count(), 0);
|
||||
// Upload still uses its existing best-effort aggregation and skips the missing rollout.
|
||||
let snapshot = CodexFeedback::new().snapshot(/*session_id*/ None);
|
||||
let uploaded = snapshot
|
||||
.feedback_attachments(
|
||||
&sentry::protocol::EnvelopeHeaders::default(),
|
||||
/*include_logs*/ false,
|
||||
Vec::new(),
|
||||
&paths,
|
||||
/*logs_override*/ None,
|
||||
)
|
||||
.collect::<Result<Vec<_>>>()?;
|
||||
assert_eq!(uploaded.len(), 1);
|
||||
assert_eq!(uploaded[0].filename, "rollouts.tar.gz");
|
||||
Ok(())
|
||||
}
|
||||
Reference in New Issue
Block a user