diff --git a/src/config/macros.rs b/src/config/macros.rs index 1da3b9d..ebee6a9 100644 --- a/src/config/macros.rs +++ b/src/config/macros.rs @@ -83,12 +83,22 @@ pub async fn macro_execute( .bootstrap_tools(app.as_ref(), true, abort_signal.clone()) .await?; - for step in ¯o_value.steps { - let command = Macro::interpolate_command(step, &variables); - println!(">> {}", multiline_text(&command)); - run_repl_command(&mut macro_ctx, abort_signal.clone(), &command).await?; + // An in-macro `.agent` step opens the top-level agent bracket on this + // fresh context, so it must also close here — on the step-failure path + // too, or the run would leak `agent.started` with no terminal event. + // Signal-aware like the headless dispatch arms: a ctrl-c'd macro + // classifies as interrupted, not failed. + let result = async { + for step in ¯o_value.steps { + let command = Macro::interpolate_command(step, &variables); + println!(">> {}", multiline_text(&command)); + run_repl_command(&mut macro_ctx, abort_signal.clone(), &command).await?; + } + Ok(()) } - Ok(()) + .await; + macro_ctx.top_level_agent_finished(result.as_ref().err(), Some(&abort_signal)); + result } struct MacroModeGuard<'a> { diff --git a/src/main.rs b/src/main.rs index 2c995e2..50d797c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -438,11 +438,12 @@ async fn run( ctx.use_agent(app.as_ref(), agent, session, abort_signal.clone()) .await?; // The top-level agent bracket does NOT open here: the inspection - // flags below (--list-sessions, --info, --macro, --rebuild-rag, - // --init-*) return early without running the agent, and an open - // bracket on those paths would leak `agent.started` with no - // terminal event. It opens at the dispatch sites further down, - // where an agent run is actually committed. + // flags below (--list-sessions, --info, --rebuild-rag, --init-*) + // return early without running the agent, and an open bracket on + // those paths would leak `agent.started` with no terminal event. + // It opens at the dispatch sites further down, where a run is + // actually committed — including the --macro arm, which executes + // rather than inspects. } else { let app: Arc = Arc::clone(&ctx.app.config); if let Some(prompt) = &cli.temp_role { @@ -575,8 +576,13 @@ async fn run( } } if let Some(name) = &cli.macro_name { - macro_execute(&mut ctx, name, text.as_deref(), abort_signal.clone()).await?; - return Ok(()); + // Dispatch committed: open the top-level agent bracket only now, + // past every inspection-flag early return above. + ctx.top_level_agent_started(); + let result = macro_execute(&mut ctx, name, text.as_deref(), abort_signal.clone()).await; + ctx.top_level_agent_finished(result.as_ref().err(), Some(&abort_signal)); + hooks::drain_pending(EXIT_HOOK_DRAIN_TIMEOUT).await; + return result; } if cli.execute && !is_repl { // Dispatch committed: open the top-level agent bracket only now, diff --git a/tests/macro_bracketing.rs b/tests/macro_bracketing.rs new file mode 100644 index 0000000..7c5594c --- /dev/null +++ b/tests/macro_bracketing.rs @@ -0,0 +1,221 @@ +//! Binary-level contract for --macro bracket symmetry: a committed macro +//! run fires agent.started plus EXACTLY ONE terminal agent event +//! (agent.completed / agent.failed / agent.interrupted), whether the agent +//! arrives via the --agent flag or an in-macro `.agent` step, in both +//! isolation modes, on success and failure alike. The markers ride on +//! global agent.* hooks admitted through the agent's `global_hooks: ["*"]` +//! wildcard, so the fixture also exercises the whitelist grammar +//! end-to-end — and stays observable even if a failing step leaves the +//! context without a loaded agent. + +use std::env; +use std::fs; +use std::path::{Path, PathBuf}; +use std::process::{Command, Output, Stdio}; +use std::thread; +use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; + +fn fresh_config_dir(label: &str) -> PathBuf { + let unique = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos(); + let tmp_dir = env::temp_dir().join(format!("coyote-macro-bracketing-{label}-{unique}")); + fs::create_dir_all(&tmp_dir).unwrap(); + tmp_dir +} + +/// Lays out a config dir with a dry_run model, global agent.* hooks that +/// append one marker line per event to `log`, a `probe-macro` agent that +/// whitelists everything, and a `probe` macro with the given YAML body. +fn write_fixture(dir: &Path, log: &Path, macro_yaml: &str) { + let log = log.display(); + // Without IS_SANDBOX (scrubbed below) a run insists on a vault password + // file, so the fixture provides one. + let vault_pass = dir.join("vault-pass"); + fs::write(&vault_pass, "test-password\n").unwrap(); + fs::write( + dir.join("config.yaml"), + format!( + "model: dryrun:dry-model\n\ + dry_run: true\n\ + vault_password_file: {vault_pass}\n\ + clients:\n\ + \x20 - type: openai\n\ + \x20 name: dryrun\n\ + \x20 auth: none\n\ + \x20 api_key: 'unused'\n\ + \x20 models:\n\ + \x20 - name: dry-model\n\ + \x20 max_input_tokens: 100000\n\ + \x20 supports_function_calling: true\n\ + save: false\n\ + memory: false\n\ + stream: false\n\ + hooks:\n\ + \x20 agent.started:\n\ + \x20 - name: mark\n\ + \x20 command: \"echo STARTED >> '{log}'\"\n\ + \x20 agent.completed:\n\ + \x20 - name: mark\n\ + \x20 command: \"echo COMPLETED >> '{log}'\"\n\ + \x20 agent.failed:\n\ + \x20 - name: mark\n\ + \x20 command: \"echo FAILED >> '{log}'\"\n\ + \x20 agent.interrupted:\n\ + \x20 - name: mark\n\ + \x20 command: \"echo INTERRUPTED >> '{log}'\"\n", + vault_pass = vault_pass.display() + ), + ) + .unwrap(); + + let agent_dir = dir.join("agents/probe-macro"); + fs::create_dir_all(&agent_dir).unwrap(); + fs::write( + agent_dir.join("config.yaml"), + "name: probe-macro\n\ + description: macro bracketing probe agent\n\ + version: 0.1.0\n\ + instructions: |\n\ + \x20 You are a probe agent. Reply briefly.\n\ + global_hooks:\n\ + \x20 - \"*\"\n", + ) + .unwrap(); + + let macros_dir = dir.join("macros"); + fs::create_dir_all(¯os_dir).unwrap(); + fs::write(macros_dir.join("probe.yaml"), macro_yaml).unwrap(); +} + +fn run_coyote(dir: &Path, args: &[&str]) -> Output { + Command::new(env!("CARGO_BIN_EXE_coyote")) + .args(args) + .current_dir(dir) + .env("COYOTE_CONFIG_DIR", dir) + .env_remove("IS_SANDBOX") + .stdin(Stdio::null()) + .output() + .unwrap() +} + +/// Bounded poll for the expected terminal marker. The binary drains pending +/// hooks before exiting, so the log is normally complete once `output()` +/// returns; the poll only absorbs filesystem latency, never paces the run. +fn wait_for_marker(log: &Path, needle: &str) -> String { + let deadline = Instant::now() + Duration::from_secs(10); + loop { + let contents = fs::read_to_string(log).unwrap_or_default(); + if contents.lines().any(|line| line == needle) { + return contents; + } + assert!( + Instant::now() < deadline, + "timed out waiting for '{needle}'; log so far: {contents:?}" + ); + thread::sleep(Duration::from_millis(50)); + } +} + +fn assert_marker_count(contents: &str, marker: &str, expected: usize) { + let count = contents.lines().filter(|line| *line == marker).count(); + assert_eq!( + count, expected, + "expected {expected}x {marker}: {contents:?}" + ); +} + +fn assert_started_and_single_terminal(contents: &str, terminal: &str) { + assert_marker_count(contents, "STARTED", 1); + for marker in ["COMPLETED", "FAILED", "INTERRUPTED"] { + let expected = usize::from(marker == terminal); + assert_marker_count(contents, marker, expected); + } +} + +fn probe_macro_run(label: &str, macro_yaml: &str, args: &[&str], expect_success: bool) -> String { + let dir = fresh_config_dir(label); + let log = dir.join("agent-events.log"); + write_fixture(&dir, &log, macro_yaml); + + let output = run_coyote(&dir, args); + assert_eq!( + output.status.success(), + expect_success, + "{label}: unexpected exit {:?}\nstdout: {}\nstderr: {}", + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + + let terminal = if expect_success { + "COMPLETED" + } else { + "FAILED" + }; + let contents = wait_for_marker(&log, terminal); + let _ = fs::remove_dir_all(&dir); + assert_started_and_single_terminal(&contents, terminal); + contents +} + +#[test] +fn agent_macro_run_fires_started_and_exactly_one_completed() { + probe_macro_run( + "agent-flag-success", + "steps:\n - \".set temperature 0.5\"\n", + &["--agent", "probe-macro", "--macro", "probe"], + true, + ); +} + +#[test] +fn agent_macro_failure_closes_the_bracket_with_exactly_one_failed() { + probe_macro_run( + "agent-flag-failure", + "steps:\n - \".agent no-such-agent-zz\"\n", + &["--agent", "probe-macro", "--macro", "probe"], + false, + ); +} + +#[test] +fn isolated_in_macro_agent_step_fires_started_and_exactly_one_completed() { + probe_macro_run( + "isolated-step-success", + "steps:\n - \".agent probe-macro\"\n - \".set temperature 0.5\"\n", + &["--macro", "probe"], + true, + ); +} + +#[test] +fn isolated_in_macro_agent_step_failure_closes_the_bracket() { + probe_macro_run( + "isolated-step-failure", + "steps:\n - \".agent probe-macro\"\n - \".agent no-such-agent-zz\"\n", + &["--macro", "probe"], + false, + ); +} + +#[test] +fn non_isolated_in_macro_agent_step_fires_started_and_exactly_one_completed() { + probe_macro_run( + "non-isolated-step-success", + "isolated: false\nsteps:\n - \".agent probe-macro\"\n - \".set temperature 0.5\"\n", + &["--macro", "probe"], + true, + ); +} + +#[test] +fn non_isolated_in_macro_agent_step_failure_closes_the_bracket() { + probe_macro_run( + "non-isolated-step-failure", + "isolated: false\nsteps:\n - \".agent probe-macro\"\n - \".agent no-such-agent-zz\"\n", + &["--macro", "probe"], + false, + ); +}