diff --git a/plugins/shape/summarize/plugin.toml b/plugins/shape/summarize/plugin.toml index 61bb773d7..7669a9408 100644 --- a/plugins/shape/summarize/plugin.toml +++ b/plugins/shape/summarize/plugin.toml @@ -2,8 +2,7 @@ name = "summarize" title = "Summaries" description = "Pseudocode summaries for large new function bodies and tests." -# Off by default: the summarizer needs an API key, and diffr stops at -# startup when a plugin that is on cannot be made. +# Off by default: the summarizer needs an API key. [enabled] title = "Summarize functions and tests" description = "Summarize large new function bodies and new tests as pseudocode. Needs an API key: `api_key`, or the provider's variable in the environment: `GEMINI_API_KEY` or `GOOGLE_API_KEY`, `OPENAI_API_KEY`, or `ANTHROPIC_API_KEY`." diff --git a/plugins/shape/summarize/plugin.wasm b/plugins/shape/summarize/plugin.wasm index aeb86c431..5bd944363 100644 Binary files a/plugins/shape/summarize/plugin.wasm and b/plugins/shape/summarize/plugin.wasm differ diff --git a/plugins/shape/summarize/rust/src/lib.rs b/plugins/shape/summarize/rust/src/lib.rs index adb46ddb0..a02c669fb 100644 --- a/plugins/shape/summarize/rust/src/lib.rs +++ b/plugins/shape/summarize/rust/src/lib.rs @@ -1,9 +1,8 @@ //! The summarizer: large new function bodies and new tests become short //! pseudocode, shown in place of the collapsed body. //! -//! It needs an API key for most providers: `new` fails without one, naming -//! how to set it or turn the plugin off, which is why the bundled -//! configuration ships it off. +//! It needs an API key for most providers: without one it does nothing and +//! says why on stderr, which is why the bundled configuration ships it off. //! Each selected body is summarized and edited in its own asynchronous callback. use anyhow::anyhow; use diffr_plugin_sdk::prelude::*; @@ -39,10 +38,18 @@ pub struct Options { /// The summarizer's options, API key and endpoint. pub struct Summarize { options: Options, - api_key: Option, + api_key: Key, endpoint: String, } +enum Key { + Present(String), + /// A custom endpoint that takes no key. + NotRequired, + /// The summarizer does nothing. + Missing, +} + impl Summarize { /// One request per selected body: its lines, numbered from `first_line` /// (1-based). Retried on transient failures; any other failure is a @@ -67,7 +74,11 @@ impl Summarize { 800, ); let url = provider.url(&self.endpoint, &self.options.model); - let headers = provider.headers(self.api_key.as_deref()); + let key = match &self.api_key { + Key::Present(key) => Some(key.as_str()), + Key::NotRequired | Key::Missing => None, + }; + let headers = provider.headers(key); let failed = |message: String| anyhow!("{}: {message}", self.options.model); let text: serde_json::Value = { let mut attempt = 0; @@ -112,31 +123,33 @@ impl Summarize { } /// The API key: the `api_key` option, or else the first of the provider's -/// environment variables that is set and not empty. `None` only where the -/// provider can go without one. -fn resolve_key(config: &Options, custom_endpoint: bool) -> anyhow::Result> { +/// environment variables that is set and not empty. `Missing` says why on +/// stderr. +fn resolve_key(config: &Options, custom_endpoint: bool) -> Key { let set = |key: &String| !key.is_empty(); if let Some(key) = config.api_key.clone().filter(set) { - return Ok(Some(key)); + return Key::Present(key); } let variables = &config.provider_details.key_variables; for variable in variables { if let Some(key) = std::env::var_os(variable) { - let key = key - .into_string() - .map_err(|_| anyhow!("{variable} is not valid UTF-8"))?; + let Ok(key) = key.into_string() else { + eprintln!("summarize: off for this run: {variable} is not valid UTF-8"); + return Key::Missing; + }; if set(&key) { - return Ok(Some(key)); + return Key::Present(key); } } } if config.provider_details.keyless_custom_endpoint && custom_endpoint { - return Ok(None); + return Key::NotRequired; } - anyhow::bail!( - "no API key: set plugins.bundled.summarize.api_key, or {} in the environment, or turn the summarizer off with plugins.bundled.summarize.enabled = false", + eprintln!( + "summarize: off for this run: no API key: set plugins.bundled.summarize.api_key, or {} in the environment, or turn the summarizer off with plugins.bundled.summarize.enabled = false", variables.join(" or ") - ) + ); + Key::Missing } impl Guest for Summarize { @@ -151,7 +164,7 @@ impl GuestPlugin for Summarize { .endpoint .clone() .filter(|endpoint| !endpoint.is_empty()); - let api_key = resolve_key(&options, endpoint.is_some()).map_err(|e| format!("{e:#}"))?; + let api_key = resolve_key(&options, endpoint.is_some()); Ok(Self { api_key, endpoint: endpoint.unwrap_or_else(|| options.provider_details.endpoint.clone()), @@ -160,6 +173,9 @@ impl GuestPlugin for Summarize { } async fn visit(&self, cursor: &Cursor, phase: Visit) -> Result { + if matches!(self.api_key, Key::Missing) { + return Ok(false); + } if phase == Visit::Post { return Ok(true); } diff --git a/src/plugin/tests/mod.rs b/src/plugin/tests/mod.rs index f987e8bcb..de2e88961 100644 --- a/src/plugin/tests/mod.rs +++ b/src/plugin/tests/mod.rs @@ -258,38 +258,6 @@ fn the_default_pipeline_makes_every_plugin_that_is_on() { ); } -#[test] -fn a_plugin_that_cannot_be_made_is_a_setup_error() { - let config = - Config::from_toml("[plugins.bundled.summarize]\nenabled = true\napi_key = ''\n").unwrap(); - // Only meaningful when the environment carries no key. - if std::env::var_os("GEMINI_API_KEY").is_some() || std::env::var_os("GOOGLE_API_KEY").is_some() - { - return; - } - let error = Pipeline::from_config(&config, Path::new("."), NonZeroUsize::MIN) - .err() - .expect("a summarizer without a key cannot be made"); - assert_eq!( - format!("{error:#}"), - "plugins.bundled.summarize: no API key: set plugins.bundled.summarize.api_key, or GEMINI_API_KEY or GOOGLE_API_KEY in the environment, or turn the summarizer off with plugins.bundled.summarize.enabled = false" - ); - if std::env::var_os("OPENAI_API_KEY").is_some() { - return; - } - let config = Config::from_toml( - "[plugins.bundled.summarize]\nenabled = true\nprovider = 'openai'\nmodel = 'm'\napi_key = ''\nendpoint = ''\n", - ) - .unwrap(); - let error = Pipeline::from_config(&config, Path::new("."), NonZeroUsize::MIN) - .err() - .expect("OpenAI at its default endpoint needs a key"); - assert_eq!( - format!("{error:#}"), - "plugins.bundled.summarize: no API key: set plugins.bundled.summarize.api_key, or OPENAI_API_KEY in the environment, or turn the summarizer off with plugins.bundled.summarize.enabled = false" - ); -} - /// The manifest can accept an option the component itself rejects: the /// component's constructor has the last word, and fails setup. #[test] diff --git a/tests/cli.rs b/tests/cli.rs index 038065523..64fdeb55c 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -71,7 +71,7 @@ fn text_output_is_gone() { } #[test] -fn a_plugin_that_cannot_be_made_stops_diffr_before_any_record() { +fn a_summarizer_without_a_key_still_diffs() { let dir = tempfile::tempdir().unwrap(); let config = dir.path().join("diffr/config.toml"); std::fs::create_dir_all(config.parent().unwrap()).unwrap(); @@ -88,13 +88,9 @@ fn a_plugin_that_cannot_be_made_stops_diffr_before_any_record() { .env("XDG_CONFIG_HOME", dir.path()) .env_remove("GEMINI_API_KEY") .env_remove("GOOGLE_API_KEY"); - cmd.assert() - .failure() - .code(2) - .stdout(predicate::str::is_empty()) - .stderr(predicate::str::contains( - "plugins.bundled.summarize: no API key: set plugins.bundled.summarize.api_key", - )); + cmd.assert().success().stderr(predicate::str::contains( + "summarize: off for this run: no API key", + )); } #[test]