From a4fb2ed7955debeb32e9f39b87ceeb42fa445839 Mon Sep 17 00:00:00 2001 From: archivr-qa Date: Sun, 23 Aug 2026 18:41:57 +0200 Subject: [PATCH] =?UTF-8?q?fix(core):=20codex=5Fcli=20=E2=80=94=20auto-dis?= =?UTF-8?q?cover=20binary=20+=20use=20--output-last-message?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related fixes for the codex_cli summary provider: 1. Executable discovery. `ARCHIVR_CODEX_CLI` was already respected, but without it the code resolved to bare `codex` and relied on PATH. The ChatGPT desktop app installs codex at `/Applications/ChatGPT.app/Contents/Resources/codex` and does not put it on PATH, so users who only have the desktop app saw 'No such file or directory' with no hint. `resolve_cli` now walks env override → a small set of well-known absolute paths → HOME /.local/bin/ → bare fallback. Same treatment applied to claude_cli for symmetry (/opt/homebrew/bin/claude, /usr/local/bin/ claude, HOME/.local/bin/claude). 2. Clean output. `codex exec -` writes a runtime header ("OpenAI Codex vX", session id, sandbox, model), the assistant reply, and a footer ("tokens used", replay of the reply) to stdout. The JSON extractor took the first '{' from the *user prompt echo* and the last '}' from the trailing replay, producing invalid text that fell through to the "raw text under summary" fallback path. Now uses `--output-last-message ` and reads only the final assistant message. Fallback (positional prompt) uses the same flag. Tempfile is cleaned up on all paths, incl. spawn failure. --- crates/archivr-core/src/summarizer.rs | 196 +++++++++++++++++++++----- 1 file changed, 159 insertions(+), 37 deletions(-) diff --git a/crates/archivr-core/src/summarizer.rs b/crates/archivr-core/src/summarizer.rs index d29ca7f..cb47f7b 100644 --- a/crates/archivr-core/src/summarizer.rs +++ b/crates/archivr-core/src/summarizer.rs @@ -163,6 +163,34 @@ fn env_timeout(name: &str, default: u64) -> u64 { .unwrap_or(default) } +/// Resolve a CLI executable path. +/// +/// Priority: `env_name` override → first `well_known_absolute` path that +/// exists → `HOME/.local/bin/` if it exists → bare name (relies on the +/// server's PATH). The macOS defaults matter for `codex`, which the ChatGPT +/// desktop app installs at `/Applications/ChatGPT.app/Contents/Resources/codex` +/// and does not add to PATH. +fn resolve_cli(env_name: &str, well_known_absolute: &[&str], bare: &str) -> PathBuf { + if let Some(explicit) = optional_env(env_name) { + return PathBuf::from(explicit); + } + for candidate in well_known_absolute { + let p = Path::new(candidate); + if p.is_file() { + return p.to_path_buf(); + } + } + if let Some(home) = env::var_os("HOME") { + let mut p = PathBuf::from(home); + p.push(".local/bin"); + p.push(bare); + if p.is_file() { + return p; + } + } + PathBuf::from(bare) +} + /// Builds a provider configuration for `kind` purely from the environment. pub fn provider_from_env(kind: &str) -> Result { match kind { @@ -179,12 +207,24 @@ pub fn provider_from_env(kind: &str) -> Result { timeout_secs: env_timeout("ARCHIVR_SUMMARY_HTTP_TIMEOUT", DEFAULT_HTTP_TIMEOUT_SECS), })), "claude_cli" => Ok(ProviderConfig::ClaudeCli(CliProviderConfig { - executable: PathBuf::from(env_or("ARCHIVR_CLAUDE_CLI", "claude")), + executable: resolve_cli( + "ARCHIVR_CLAUDE_CLI", + &["/opt/homebrew/bin/claude", "/usr/local/bin/claude"], + "claude", + ), model: optional_env("ARCHIVR_CLAUDE_MODEL"), timeout_secs: env_timeout("ARCHIVR_SUMMARY_CLI_TIMEOUT", DEFAULT_CLI_TIMEOUT_SECS), })), "codex_cli" => Ok(ProviderConfig::CodexCli(CliProviderConfig { - executable: PathBuf::from(env_or("ARCHIVR_CODEX_CLI", "codex")), + executable: resolve_cli( + "ARCHIVR_CODEX_CLI", + &[ + "/Applications/ChatGPT.app/Contents/Resources/codex", + "/opt/homebrew/bin/codex", + "/usr/local/bin/codex", + ], + "codex", + ), model: optional_env("ARCHIVR_CODEX_MODEL"), timeout_secs: env_timeout("ARCHIVR_SUMMARY_CLI_TIMEOUT", DEFAULT_CLI_TIMEOUT_SECS), })), @@ -448,48 +488,114 @@ impl SummaryProvider for ClaudeCliProvider { /// Codex invocation lives in its own module because its one-shot interface is /// the least stable of the four. /// -/// Primary form is `codex exec -`, which reads the prompt from stdin. Older -/// builds only accept the prompt as a positional argument, so a failure to -/// spawn/parse falls back to `codex exec `. -/// -/// TESTED: neither form was exercised end-to-end — `codex` is not installed on -/// the machine this was written on (`which codex` → not found). The `claude` -/// CLI path *was* smoke-tested. Treat the codex path as best-effort until -/// someone with the binary confirms it. +/// Primary form is `codex exec --output-last-message -`, which reads the +/// prompt from stdin and writes ONLY the final assistant message to ``. +/// Without `--output-last-message`, stdout is polluted with a header +/// (`OpenAI Codex vX`, session id, model, sandbox, …) and a footer +/// (`tokens used`, message replay), and the JSON extractor can pick up the +/// echoed user prompt instead of the real answer. Older builds that reject +/// `-` as stdin marker fall back to a positional prompt. mod codex { use super::*; + /// A short-lived path in the OS temp dir. Unique per (pid, wall time) so + /// concurrent summarizations don't collide. + fn last_message_temp_path() -> PathBuf { + let stamp = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or_default(); + env::temp_dir().join(format!( + "archivr-codex-{}-{}.txt", + std::process::id(), + stamp + )) + } + + fn missing_binary_hint(cfg: &CliProviderConfig) -> &'static str { + // Only nudge users about the env var when we're running the default + // bare "codex" and it failed — an explicit ARCHIVR_CODEX_CLI path + // failure is their configuration, not a discovery gap. + if cfg.executable == Path::new("codex") { + " (hint: set ARCHIVR_CODEX_CLI to your codex binary; on macOS the ChatGPT desktop app installs it at /Applications/ChatGPT.app/Contents/Resources/codex)" + } else { + "" + } + } + + fn read_and_cleanup(path: &Path) -> Option { + let out = std::fs::read_to_string(path).ok()?; + let _ = std::fs::remove_file(path); + let trimmed = out.trim(); + if trimmed.is_empty() { None } else { Some(trimmed.to_string()) } + } + pub fn run(cfg: &CliProviderConfig, prompt: &str) -> Result { - let mut args: Vec = vec!["exec".into(), "-".into()]; + let out_path = last_message_temp_path(); + let out_str = out_path.to_string_lossy().into_owned(); + + // Primary: stdin prompt + --output-last-message. + let mut primary: Vec = vec![ + "exec".into(), + "--output-last-message".into(), + out_str.clone(), + ]; if let Some(model) = cfg.model.as_deref() { - args.push("--model".into()); - args.push(model.into()); + primary.push("--model".into()); + primary.push(model.into()); } - let arg_refs: Vec<&str> = args.iter().map(String::as_str).collect(); - match run_cli(&cfg.executable, &arg_refs, prompt, cfg.timeout_secs) { - Ok(out) => Ok(out), - Err(primary) => { - // Fallback: prompt as a positional argument, no stdin. - let mut fb: Vec = vec!["exec".into()]; - if let Some(model) = cfg.model.as_deref() { - fb.push("--model".into()); - fb.push(model.into()); + primary.push("-".into()); + let primary_refs: Vec<&str> = primary.iter().map(String::as_str).collect(); + + let primary_err = match run_cli(&cfg.executable, &primary_refs, prompt, cfg.timeout_secs) { + Ok(_) => { + if let Some(text) = read_and_cleanup(&out_path) { + return Ok(text); } - fb.push(prompt.into()); - let out = Command::new(&cfg.executable) - .args(&fb) - .output() - .with_context(|| format!("codex `exec -` failed ({primary:#}); positional fallback also failed to spawn"))?; - if !out.status.success() { - bail!( - "codex `exec -` failed ({primary:#}); positional fallback exited with {}: {}", - out.status, - truncate_for_error(&String::from_utf8_lossy(&out.stderr)) - ); - } - Ok(String::from_utf8_lossy(&out.stdout).to_string()) + // Codex succeeded but wrote nothing to the file — extremely + // rare, but treat as a soft failure so we try the fallback. + anyhow!("codex produced no last-message output") } + Err(e) => { + let _ = std::fs::remove_file(&out_path); + e + } + }; + + // Fallback: positional prompt, no stdin, same --output-last-message. + let mut fb: Vec = vec![ + "exec".into(), + "--output-last-message".into(), + out_str.clone(), + ]; + if let Some(model) = cfg.model.as_deref() { + fb.push("--model".into()); + fb.push(model.into()); } + fb.push(prompt.into()); + let out = Command::new(&cfg.executable) + .args(&fb) + .output() + .with_context(|| { + format!( + "codex `exec -` failed ({primary_err:#}); positional fallback also failed to spawn{}", + missing_binary_hint(cfg) + ) + })?; + if !out.status.success() { + let _ = std::fs::remove_file(&out_path); + bail!( + "codex `exec -` failed ({primary_err:#}); positional fallback exited with {}: {}", + out.status, + truncate_for_error(&String::from_utf8_lossy(&out.stderr)) + ); + } + if let Some(text) = read_and_cleanup(&out_path) { + return Ok(text); + } + // Last resort — the child succeeded but wrote nothing to the file. Fall + // back to raw stdout so the caller has *something* to normalize. + Ok(String::from_utf8_lossy(&out.stdout).to_string()) } } @@ -902,14 +1008,30 @@ mod tests { let ProviderConfig::ClaudeCli(c) = provider_from_env("claude_cli").unwrap() else { panic!("wrong variant") }; - assert_eq!(c.executable, PathBuf::from("claude")); + // Same rationale as the codex case below: resolve_cli may discover a + // well-known install path, so accept either the bare name or any + // file-name-`claude` path. + assert!( + c.executable == PathBuf::from("claude") + || c.executable.file_name().map(|f| f == "claude").unwrap_or(false), + "unexpected claude executable: {}", + c.executable.display() + ); assert_eq!(c.model, None); assert_eq!(c.timeout_secs, DEFAULT_CLI_TIMEOUT_SECS); let ProviderConfig::CodexCli(c) = provider_from_env("codex_cli").unwrap() else { panic!("wrong variant") }; - assert_eq!(c.executable, PathBuf::from("codex")); + // Either the well-known ChatGPT.app path (if present on this host) or + // the bare `codex` fallback is acceptable — `resolve_cli` is + // deliberately opportunistic. + assert!( + c.executable == PathBuf::from("codex") + || c.executable.file_name().map(|f| f == "codex").unwrap_or(false), + "unexpected codex executable: {}", + c.executable.display() + ); } #[test]