feat(TASK-050): bracket --macro runs like --execute and close the isolated-macro leak
The --macro dispatch arm now opens the top-level agent bracket before macro_execute and closes it signal-aware afterwards, draining pending hooks like the --execute arm; both bracket calls no-op without a loaded agent, so agent-less macro runs are unaffected. The isolated arm of macro_execute closes the bracket an in-macro .agent step opens on its fresh context, on the step-failure path too. Binary-level tests pin agent.started plus exactly one terminal event for the --agent flag and for in-macro .agent steps in both isolation modes, on success and failure.
This commit is contained in:
3 files changed
+244
-7
No files matched your search
@@ -83,12 +83,22 @@ pub async fn macro_execute(
|
|||||||
.bootstrap_tools(app.as_ref(), true, abort_signal.clone())
|
.bootstrap_tools(app.as_ref(), true, abort_signal.clone())
|
||||||
.await?;
|
.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 {
|
for step in ¯o_value.steps {
|
||||||
let command = Macro::interpolate_command(step, &variables);
|
let command = Macro::interpolate_command(step, &variables);
|
||||||
println!(">> {}", multiline_text(&command));
|
println!(">> {}", multiline_text(&command));
|
||||||
run_repl_command(&mut macro_ctx, abort_signal.clone(), &command).await?;
|
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> {
|
struct MacroModeGuard<'a> {
|
||||||
|
|||||||
+13
-7
@@ -438,11 +438,12 @@ async fn run(
|
|||||||
ctx.use_agent(app.as_ref(), agent, session, abort_signal.clone())
|
ctx.use_agent(app.as_ref(), agent, session, abort_signal.clone())
|
||||||
.await?;
|
.await?;
|
||||||
// The top-level agent bracket does NOT open here: the inspection
|
// The top-level agent bracket does NOT open here: the inspection
|
||||||
// flags below (--list-sessions, --info, --macro, --rebuild-rag,
|
// flags below (--list-sessions, --info, --rebuild-rag, --init-*)
|
||||||
// --init-*) return early without running the agent, and an open
|
// return early without running the agent, and an open bracket on
|
||||||
// bracket on those paths would leak `agent.started` with no
|
// those paths would leak `agent.started` with no terminal event.
|
||||||
// terminal event. It opens at the dispatch sites further down,
|
// It opens at the dispatch sites further down, where a run is
|
||||||
// where an agent run is actually committed.
|
// actually committed — including the --macro arm, which executes
|
||||||
|
// rather than inspects.
|
||||||
} else {
|
} else {
|
||||||
let app: Arc<AppConfig> = Arc::clone(&ctx.app.config);
|
let app: Arc<AppConfig> = Arc::clone(&ctx.app.config);
|
||||||
if let Some(prompt) = &cli.temp_role {
|
if let Some(prompt) = &cli.temp_role {
|
||||||
@@ -575,8 +576,13 @@ async fn run(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
if let Some(name) = &cli.macro_name {
|
if let Some(name) = &cli.macro_name {
|
||||||
macro_execute(&mut ctx, name, text.as_deref(), abort_signal.clone()).await?;
|
// Dispatch committed: open the top-level agent bracket only now,
|
||||||
return Ok(());
|
// 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 {
|
if cli.execute && !is_repl {
|
||||||
// Dispatch committed: open the top-level agent bracket only now,
|
// Dispatch committed: open the top-level agent bracket only now,
|
||||||
|
|||||||
@@ -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,
|
||||||
|
);
|
||||||
|
}
|
||||||
Reference in new issue
Block a user