From 3291463ff16c2f15504ed7430a9acadc919da76b Mon Sep 17 00:00:00 2001 From: Abhinav Date: Tue, 28 Apr 2026 15:45:26 -0700 Subject: [PATCH] Fix flaky plugin hook env test (#20088) The test was flaky because it was checking the right thing in a roundabout way. What it wanted to prove: - plugin hooks receive the right environment variables. What it actually did: 1. Run a plugin hook. 2. Have that hook write those env vars into a temporary `env.json` file. 3. After the hook finished, read `env.json` back from disk. On Windows, that last file was sometimes not there when the test tried to read it, so the test failed with `read env log: file not found`. The hook system itself was not what the test failure was directly proving; the test was failing on the extra filesystem side effect it introduced. The fix is to stop using a temp file as the proof mechanism. The hook now prints the env values in its normal structured output, and the test asserts on the output that the hook engine already captures. So we still verify the same behavior, but without depending on a separate file being created and read back correctly on Windows. --- codex-rs/hooks/src/engine/mod_tests.rs | 30 +++++++++++++++----------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/codex-rs/hooks/src/engine/mod_tests.rs b/codex-rs/hooks/src/engine/mod_tests.rs index b29542d8b..06ec3b251 100644 --- a/codex-rs/hooks/src/engine/mod_tests.rs +++ b/codex-rs/hooks/src/engine/mod_tests.rs @@ -19,6 +19,8 @@ use codex_config::TomlValue; use codex_plugin::PluginHookSource; use codex_plugin::PluginId; use codex_protocol::ThreadId; +use codex_protocol::protocol::HookOutputEntryKind; +use codex_protocol::protocol::HookRunStatus; use codex_protocol::protocol::HookSource; use pretty_assertions::assert_eq; use tempfile::tempdir; @@ -345,22 +347,18 @@ async fn plugin_hook_sources_run_with_plugin_env_and_plugin_source() { AbsolutePathBuf::try_from(temp.path().join("plugin-data")).expect("plugin data root"); fs::create_dir_all(plugin_root.join("hooks")).expect("create hooks dir"); let source_path = plugin_root.join("hooks/hooks.json"); - let log_path = plugin_root.join("env.json"); let script_path = plugin_root.join("hooks/write_env.py"); fs::write( script_path.as_path(), - format!( - r#"import json + r#"import json import os -from pathlib import Path - -Path(r"{log_path}").write_text(json.dumps({{ - "plugin": os.environ.get("PLUGIN_ROOT"), - "claude": os.environ.get("CLAUDE_PLUGIN_ROOT"), -}}), encoding="utf-8") +print(json.dumps({ + "systemMessage": json.dumps({ + "plugin": os.environ.get("PLUGIN_ROOT"), + "claude": os.environ.get("CLAUDE_PLUGIN_ROOT"), + }) +})) "#, - log_path = log_path.display(), - ), ) .expect("write hook script"); let plugin_id = PluginId::parse("demo-plugin@test-marketplace").expect("plugin id"); @@ -427,9 +425,15 @@ Path(r"{log_path}").write_text(json.dumps({{ assert_eq!(outcome.hook_events.len(), 1); assert_eq!(outcome.hook_events[0].run.source, HookSource::Plugin); + assert_eq!(outcome.hook_events[0].run.status, HookRunStatus::Completed); + assert_eq!(outcome.hook_events[0].run.entries.len(), 1); + assert_eq!( + outcome.hook_events[0].run.entries[0].kind, + HookOutputEntryKind::Warning + ); let logged: serde_json::Value = - serde_json::from_str(&fs::read_to_string(log_path.as_path()).expect("read env log")) - .expect("parse env log"); + serde_json::from_str(&outcome.hook_events[0].run.entries[0].text) + .expect("parse env payload"); assert_eq!( logged, serde_json::json!({