diff --git a/codex-rs/cli/src/plugin_cmd.rs b/codex-rs/cli/src/plugin_cmd.rs index d2824526e7..e32c1d8d19 100644 --- a/codex-rs/cli/src/plugin_cmd.rs +++ b/codex-rs/cli/src/plugin_cmd.rs @@ -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::>(); + write_plugin_list(&mut io::stdout().lock(), marketplaces, args) +} + +fn write_plugin_list( + writer: &mut impl Write, + marketplaces: Vec, + 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::() + .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, + 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, "{: bool { .collect::>(), ) } + +#[cfg(test)] +#[path = "plugin_cmd_tests.rs"] +mod tests; diff --git a/codex-rs/cli/src/plugin_cmd_tests.rs b/codex-rs/cli/src/plugin_cmd_tests.rs new file mode 100644 index 0000000000..db49273d84 --- /dev/null +++ b/codex-rs/cli/src/plugin_cmd_tests.rs @@ -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 { + 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::() + .unwrap(); + assert_eq!(error.kind(), kind); + assert_eq!(error.to_string(), "output failed"); + } + } + } +} diff --git a/codex-rs/cli/tests/plugin_cli.rs b/codex-rs/cli/tests/plugin_cli.rs index b1c316860c..ea80f55a84 100644 --- a/codex-rs/cli/tests/plugin_cli.rs +++ b/codex-rs/cli/tests/plugin_cli.rs @@ -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::>(); + 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()?;