mirror of
https://github.com/openai/codex.git
synced 2026-10-20 17:38:45 +00:00
Handle broken pipes gracefully in plugin list output (#53109)
## Why Piping `codex plugin list` into a consumer such as `head` can close stdout before the full catalog is written, causing a panic. ## What changed Write both table and JSON output through a fallible writer and treat broken pipes during writing or flushing as success. Continue propagating other output errors. ## Testing Add unit tests for write and flush failures in both output formats, plus CLI tests that close stdout early and verify a successful exit without a panic. GitOrigin-RevId: cc4a64e10ad723d26b6de9e731efdc534dbee492
This commit is contained in:
@@ -37,6 +37,8 @@ use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_cli::CliConfigOverrides;
|
||||
use serde::Serialize;
|
||||
use std::collections::HashMap;
|
||||
use std::io;
|
||||
use std::io::Write;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
@@ -307,17 +309,49 @@ pub async fn run_plugin_list(
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
write_plugin_list(&mut io::stdout().lock(), marketplaces, args)
|
||||
}
|
||||
|
||||
fn write_plugin_list(
|
||||
writer: &mut impl Write,
|
||||
marketplaces: Vec<PluginListMarketplace>,
|
||||
args: ListPluginsArgs,
|
||||
) -> Result<()> {
|
||||
let result = render_plugin_list(writer, marketplaces, args)
|
||||
.and_then(|()| writer.flush().map_err(Into::into));
|
||||
match result {
|
||||
// A consumer such as `head` may finish before the full catalog is written.
|
||||
Err(error)
|
||||
if error
|
||||
.downcast_ref::<io::Error>()
|
||||
.is_some_and(|error| error.kind() == io::ErrorKind::BrokenPipe) =>
|
||||
{
|
||||
Ok(())
|
||||
}
|
||||
Ok(()) => Ok(()),
|
||||
Err(error) => Err(error),
|
||||
}
|
||||
}
|
||||
|
||||
fn render_plugin_list(
|
||||
writer: &mut impl Write,
|
||||
marketplaces: Vec<PluginListMarketplace>,
|
||||
args: ListPluginsArgs,
|
||||
) -> Result<()> {
|
||||
if args.json {
|
||||
let output = JsonPluginListOutput::from_marketplaces(marketplaces, args.available);
|
||||
println!("{}", serde_json::to_string_pretty(&output)?);
|
||||
writeln!(writer, "{}", serde_json::to_string_pretty(&output)?)?;
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
if marketplaces.is_empty() {
|
||||
if let Some(marketplace_name) = args.marketplace_name {
|
||||
println!("No plugins found in marketplace `{marketplace_name}`.");
|
||||
writeln!(
|
||||
writer,
|
||||
"No plugins found in marketplace `{marketplace_name}`."
|
||||
)?;
|
||||
} else {
|
||||
println!("No marketplace plugins found.");
|
||||
writeln!(writer, "No marketplace plugins found.")?;
|
||||
}
|
||||
} else {
|
||||
for (index, marketplace) in marketplaces.into_iter().enumerate() {
|
||||
@@ -376,23 +410,25 @@ pub async fn run_plugin_list(
|
||||
}
|
||||
|
||||
if index > 0 {
|
||||
println!();
|
||||
writeln!(writer)?;
|
||||
}
|
||||
println!("Marketplace `{}`", marketplace.name);
|
||||
writeln!(writer, "Marketplace `{}`", marketplace.name)?;
|
||||
if let Some(path) = &marketplace.path {
|
||||
println!("{}", path.display());
|
||||
writeln!(writer, "{}", path.display())?;
|
||||
} else {
|
||||
println!("Remote catalog");
|
||||
writeln!(writer, "Remote catalog")?;
|
||||
}
|
||||
println!();
|
||||
println!(
|
||||
writeln!(writer)?;
|
||||
writeln!(
|
||||
writer,
|
||||
"{:<plugin_width$} {:<status_width$} {:<installed_version_width$} SOURCE",
|
||||
"PLUGIN", "STATUS", "VERSION"
|
||||
);
|
||||
)?;
|
||||
for (plugin, status, installed_version, path) in rows {
|
||||
println!(
|
||||
writeln!(
|
||||
writer,
|
||||
"{plugin:<plugin_width$} {status:<status_width$} {installed_version:<installed_version_width$} {path}"
|
||||
);
|
||||
)?;
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1090,3 +1126,7 @@ fn path_ends_with(path: &Path, suffix: &[&str]) -> bool {
|
||||
.collect::<Vec<_>>(),
|
||||
)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "plugin_cmd_tests.rs"]
|
||||
mod tests;
|
||||
|
||||
@@ -0,0 +1,76 @@
|
||||
//! Verify plugin-list output error handling at the writer boundary.
|
||||
|
||||
use super::ListPluginsArgs;
|
||||
use super::write_plugin_list;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::io;
|
||||
use std::io::Write;
|
||||
|
||||
#[derive(Clone, Copy)]
|
||||
enum Failure {
|
||||
Write,
|
||||
Flush,
|
||||
}
|
||||
|
||||
struct FailingWriter {
|
||||
failure: Failure,
|
||||
kind: io::ErrorKind,
|
||||
}
|
||||
|
||||
impl Write for FailingWriter {
|
||||
fn write(&mut self, bytes: &[u8]) -> io::Result<usize> {
|
||||
match self.failure {
|
||||
Failure::Write => Err(io::Error::new(self.kind, "output failed")),
|
||||
Failure::Flush => Ok(bytes.len()),
|
||||
}
|
||||
}
|
||||
|
||||
fn flush(&mut self) -> io::Result<()> {
|
||||
match self.failure {
|
||||
Failure::Write => Ok(()),
|
||||
Failure::Flush => Err(io::Error::new(self.kind, "output failed")),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn plugin_list_treats_broken_pipe_as_success() {
|
||||
for json in [false, true] {
|
||||
for failure in [Failure::Write, Failure::Flush] {
|
||||
let mut writer = FailingWriter {
|
||||
failure,
|
||||
kind: io::ErrorKind::BrokenPipe,
|
||||
};
|
||||
let args = ListPluginsArgs {
|
||||
marketplace_name: None,
|
||||
json,
|
||||
available: true,
|
||||
};
|
||||
|
||||
write_plugin_list(&mut writer, Vec::new(), args).unwrap();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn plugin_list_preserves_other_output_errors() {
|
||||
for json in [false, true] {
|
||||
for failure in [Failure::Write, Failure::Flush] {
|
||||
for kind in [io::ErrorKind::PermissionDenied, io::ErrorKind::WriteZero] {
|
||||
let mut writer = FailingWriter { failure, kind };
|
||||
let args = ListPluginsArgs {
|
||||
marketplace_name: None,
|
||||
json,
|
||||
available: true,
|
||||
};
|
||||
|
||||
let error = write_plugin_list(&mut writer, Vec::new(), args)
|
||||
.unwrap_err()
|
||||
.downcast::<io::Error>()
|
||||
.unwrap();
|
||||
assert_eq!(error.kind(), kind);
|
||||
assert_eq!(error.to_string(), "output failed");
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1,3 +1,4 @@
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
use anyhow::ensure;
|
||||
use app_test_support::ChatGptAuthFixture;
|
||||
@@ -16,10 +17,12 @@ use serde_json::Value;
|
||||
use serde_json::json;
|
||||
use std::path::Path;
|
||||
use std::process::Output;
|
||||
use std::process::Stdio;
|
||||
use std::sync::Arc;
|
||||
use std::sync::atomic::AtomicBool;
|
||||
use std::sync::atomic::Ordering;
|
||||
use tempfile::TempDir;
|
||||
use tokio::io::AsyncReadExt;
|
||||
use tokio::process::Command;
|
||||
use wiremock::Mock;
|
||||
use wiremock::MockServer;
|
||||
@@ -638,6 +641,64 @@ async fn plugin_list_prints_plugins_in_a_table() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn plugin_list_succeeds_when_stdout_closes_early() -> Result<()> {
|
||||
assert_plugin_list_succeeds_when_stdout_closes_early(&[]).await
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn plugin_list_json_succeeds_when_stdout_closes_early() -> Result<()> {
|
||||
assert_plugin_list_succeeds_when_stdout_closes_early(&["--json", "--available"]).await
|
||||
}
|
||||
|
||||
async fn assert_plugin_list_succeeds_when_stdout_closes_early(args: &[&str]) -> Result<()> {
|
||||
let (codex_home, source) = setup_local_marketplace()?;
|
||||
let plugins = (0..32_768)
|
||||
.map(|index| {
|
||||
json!({
|
||||
"name": format!("sample-{index:05}"),
|
||||
"source": {"source": "npm", "package": "sample"},
|
||||
})
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
write_marketplace_source_with_manifest(
|
||||
source.path(),
|
||||
&json!({"name": "debug", "plugins": plugins}).to_string(),
|
||||
)?;
|
||||
|
||||
// The producer must not fit its entire output in the pipe before we close it.
|
||||
let complete = codex_command(codex_home.path())?
|
||||
.args(["plugin", "list", "--marketplace", "debug"])
|
||||
.args(args)
|
||||
.assert()
|
||||
.success();
|
||||
assert!(complete.get_output().stdout.len() > 1024 * 1024);
|
||||
|
||||
let mut child = Command::new(codex_utils_cargo_bin::cargo_bin("codex")?)
|
||||
.env("CODEX_HOME", codex_home.path())
|
||||
.env("HOME", codex_home.path())
|
||||
.current_dir(codex_home.path())
|
||||
.args(["plugin", "list", "--marketplace", "debug"])
|
||||
.args(args)
|
||||
.stdout(Stdio::piped())
|
||||
.stderr(Stdio::piped())
|
||||
.spawn()?;
|
||||
let mut stdout = child.stdout.take().context("piped stdout")?;
|
||||
let mut prefix = [0; 1];
|
||||
stdout.read_exact(&mut prefix).await?;
|
||||
drop(stdout);
|
||||
|
||||
let output = child.wait_with_output().await?;
|
||||
assert!(
|
||||
output.status.success(),
|
||||
"plugin list {args:?} exited with {}: {}",
|
||||
output.status,
|
||||
String::from_utf8_lossy(&output.stderr),
|
||||
);
|
||||
assert!(!String::from_utf8(output.stderr)?.contains("panicked"));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn plugin_list_json_prints_available_plugins_when_requested() -> Result<()> {
|
||||
let (codex_home, source) = setup_local_marketplace()?;
|
||||
|
||||
Reference in New Issue
Block a user