From 9da0b916730126d90b89c089eaed4a72a4ce2c41 Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Wed, 16 Sep 2026 15:07:21 -0600 Subject: [PATCH] fix(TASK-046): make inspection flags look-don't-touch and pin the contracts - Skip the builtins bootstrap, function-binary build, and tool-scope rebuild for inspection-only runs (--info/--list-*/--mcp-list/ --list-secrets without --agent/--role): a pristine COYOTE_CONFIG_DIR now stays byte-empty (was 269 files written before dispatch) - Tolerate a model-less default config on the inspection surface via AppConfig::from_config_lenient + Model::default() in bootstrap; strict 'No available model' behavior unchanged for the REPL path and all mutating mcp/vault flags - --list-secrets degrades to an rc=0 informational message in sandbox mode (mutating vault flags still fail there), pinned by test - Add the 041.7-4 regression test: a malformed COYOTE__GLOBAL_TOOLS env override logs the debug message and leaves global_tools untouched - Adopt scripts/usage-probe-inspection.sh (10 flags: rc=0 + zero writes, output required only for --info) and add binary-level tests/inspection_flags.rs, incl. the agent+--info still-bootstraps pin --- scripts/usage-probe-inspection.sh | 92 ++++++++++++++++++++++ src/config/agent.rs | 27 +++++++ src/config/app_config.rs | 32 +++++++- src/config/app_state.rs | 9 ++- src/config/request_context.rs | 8 +- src/main.rs | 57 +++++++++++--- src/testing.rs | 42 +++++----- src/vault/mod.rs | 29 ++++++- tests/inspection_flags.rs | 124 ++++++++++++++++++++++++++++++ 9 files changed, 389 insertions(+), 31 deletions(-) create mode 100755 scripts/usage-probe-inspection.sh create mode 100644 tests/inspection_flags.rs diff --git a/scripts/usage-probe-inspection.sh b/scripts/usage-probe-inspection.sh new file mode 100755 index 0000000..5be12cb --- /dev/null +++ b/scripts/usage-probe-inspection.sh @@ -0,0 +1,92 @@ +#!/usr/bin/env bash +# Black-box usage-pattern probe for inspection flags on a pristine config dir. +# +# Contract under test (first-run wizard bypass for look-don't-touch flags): +# With COYOTE_CONFIG_DIR pointing at an EMPTY directory, every inspection +# flag (--info, --list-* , --mcp-list, vault list) must: +# 1) exit without prompting (no tty read, no first-run wizard), +# 2) exit 0 (Config::default() fallback); only --info guarantees output +# on stdout — list flags legitimately emit an empty listing on a +# pristine dir, +# 3) write NOTHING into the config dir (no config.yaml, no builtins). +# The plain interactive path (`coyote` on a tty, no flags) must still reach +# the first-run wizard prompt; that is covered manually / by the REPL recipe +# and is intentionally not asserted here (needs a pty). +# +# Usage: scripts/usage-probe-inspection.sh +# Requires: a `coyote` binary on PATH or at ./target/debug/coyote or +# ./target/release/coyote (build with `cargo build --bin coyote` first). + +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +BIN="${COYOTE_BIN:-}" +if [ -z "$BIN" ]; then + if [ -x "$REPO_ROOT/target/debug/coyote" ]; then + BIN="$REPO_ROOT/target/debug/coyote" + elif [ -x "$REPO_ROOT/target/release/coyote" ]; then + BIN="$REPO_ROOT/target/release/coyote" + else + echo "No coyote binary found; run 'cargo build --bin coyote' first." >&2 + exit 2 + fi +fi + +WORKDIR="$(mktemp -d)" +trap 'rm -rf "$WORKDIR"' EXIT + +failures=0 +fail() { + echo "FAIL: $1" >&2 + failures=$((failures + 1)) +} + +# Each flag gets a fresh, empty config dir. stdin is closed so any attempt to +# prompt fails fast; timeout guards against a hang instead of a read error. +probe_flag() { + # $1 = "require-output" or "allow-empty" (stdout expectation), + # $2... = the flag (and any args) to pass + local output_mode="$1"; shift + local desc="$*" + local cfg out rc nwritten + cfg="$(mktemp -d "$WORKDIR/cfg.XXXXXX")" + out="$WORKDIR/out.txt" + rc=0 + COYOTE_CONFIG_DIR="$cfg" timeout 30 "$BIN" "$@" "$out" 2>&1 || rc=$? + + if [ "$rc" = "124" ]; then + fail "$desc: hung (likely waiting on a prompt) on an empty config dir" + return + fi + if [ "$rc" != "0" ]; then + fail "$desc: expected clean exit 0 on an empty config dir, got rc=$rc, output: $(head -c 200 "$out")" + fi + if [ "$output_mode" = "require-output" ] && [ ! -s "$out" ]; then + fail "$desc: expected some output, got none" + fi + nwritten="$(find "$cfg" -mindepth 1 | wc -l | tr -d ' ')" + if [ "$nwritten" != "0" ]; then + fail "$desc: expected NOTHING written into the empty config dir, found $nwritten entries: $(find "$cfg" -mindepth 1 -maxdepth 1 | tr '\n' ' ')" + fi +} + +echo "== Inspection flags on a pristine COYOTE_CONFIG_DIR: no wizard, clean exit, zero writes ==" +probe_flag require-output --info +probe_flag allow-empty --list-models +probe_flag allow-empty --list-roles +probe_flag allow-empty --list-sessions +probe_flag allow-empty --list-agents +probe_flag allow-empty --list-macros +probe_flag allow-empty --list-skills +probe_flag allow-empty --list-bundles +probe_flag allow-empty --list-secrets +probe_flag allow-empty --mcp-list + +if [ "$failures" != "0" ]; then + echo + echo "$failures inspection-flag scenario(s) FAILED" >&2 + exit 1 +fi + +echo +echo "ALL SCENARIOS PASSED" diff --git a/src/config/agent.rs b/src/config/agent.rs index a76b9c1..00ed166 100644 --- a/src/config/agent.rs +++ b/src/config/agent.rs @@ -1475,6 +1475,33 @@ global_hooks: } } + #[test] + #[serial_test::serial] + fn load_envs_ignores_malformed_global_tools_and_logs_debug() { + crate::testing::install_log_collector(); + let name = "gt-malformed-envtest"; + let yaml = format!("name: {name}\ninstructions: hi\nglobal_tools:\n - keep_tool.sh\n"); + let mut config: AgentConfig = serde_yaml::from_str(&yaml).unwrap(); + let env_name = normalize_env_name(&format!("{name}_global_tools")); + let _guard = crate::testing::EnvVarGuard::set(&env_name, "{not-json"); + + config.load_envs(&AppConfig::default()); + + assert_eq!( + config.global_tools, + vec!["keep_tool.sh"], + "a malformed override must leave the existing value untouched" + ); + let debugs = crate::testing::debug_snapshot(); + assert!( + debugs.iter().any(|message| { + message.contains("Ignoring malformed global_tools env override") + && message.contains(name) + }), + "expected a debug log for the malformed override: {debugs:?}" + ); + } + #[test] fn from_graph_carries_hooks_and_global_hooks() { let yaml = "\ diff --git a/src/config/app_config.rs b/src/config/app_config.rs index bfa2fb9..6fe6ddd 100644 --- a/src/config/app_config.rs +++ b/src/config/app_config.rs @@ -210,6 +210,18 @@ impl Default for AppConfig { impl AppConfig { pub fn from_config(config: super::Config) -> Result { + Self::from_config_impl(config, true) + } + + /// Like [`Self::from_config`], but tolerates a config with no resolvable + /// model: `model_id` stays empty instead of failing with "No available + /// model". For inspection readouts (--info, --list-*, --mcp-list, + /// --list-secrets), which render model fields as absent/default. + pub fn from_config_lenient(config: super::Config) -> Result { + Self::from_config_impl(config, false) + } + + fn from_config_impl(config: super::Config, require_model: bool) -> Result { let mut app_config = Self { model_id: config.model_id, temperature: config.temperature, @@ -306,7 +318,13 @@ impl AppConfig { } app_config.setup_document_loaders(); app_config.setup_user_agent(); - app_config.resolve_model()?; + if require_model { + app_config.resolve_model()?; + } else { + // Model-less is fine here: model_id stays empty and inspection + // callers substitute Model::default(). + let _ = app_config.resolve_model(); + } app_config.validate_reasoning_effort()?; Ok(app_config) } @@ -1269,6 +1287,18 @@ mod tests { assert!(result.is_err()); } + #[test] + fn from_config_errors_when_no_model_available() { + let result = AppConfig::from_config(Config::default()); + assert!(result.is_err()); + } + + #[test] + fn from_config_lenient_tolerates_missing_model() { + let app = AppConfig::from_config_lenient(Config::default()).unwrap(); + assert!(app.model_id.is_empty()); + } + #[test] fn resolve_model_keeps_explicit_model_id() { let mut app = AppConfig { diff --git a/src/config/app_state.rs b/src/config/app_state.rs index 4190b49..29d20e4 100644 --- a/src/config/app_state.rs +++ b/src/config/app_state.rs @@ -43,6 +43,7 @@ impl AppState { log_path: Option, start_mcp_servers: bool, lenient_vault: bool, + inspection: bool, abort_signal: AbortSignal, ) -> Result { let vault = if lenient_vault { @@ -74,7 +75,13 @@ impl AppState { } } - let mut functions = Functions::init(config.visible_tools.as_deref())?; + // Inspection-only runs are look-don't-touch readouts: skip the + // function-binary prune/build, which writes into the config dir. + let mut functions = if inspection { + Functions::default() + } else { + Functions::init(config.visible_tools.as_deref())? + }; if !mcp_registry.is_empty() && config.mcp_server_support { functions.append_mcp_meta_functions(mcp_registry.server_features()); } diff --git a/src/config/request_context.rs b/src/config/request_context.rs index 8874ed8..3e3e412 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -417,7 +417,13 @@ impl RequestContext { working_mode: WorkingMode, info_flag: bool, ) -> Result { - let model = Model::retrieve_model(&app.config, &app.config.model_id, ModelType::Chat)?; + // A lenient AppConfig (inspection-only run, no resolvable model) + // leaves model_id empty; render those readouts with a default model. + let model = if info_flag && app.config.model_id.is_empty() { + Model::default() + } else { + Model::retrieve_model(&app.config, &app.config.model_id, ModelType::Chat)? + }; let mut functions = app.functions.clone(); if working_mode.is_repl() { diff --git a/src/main.rs b/src/main.rs index 52ff77a..012c511 100644 --- a/src/main.rs +++ b/src/main.rs @@ -140,7 +140,16 @@ async fn main() -> Result<()> { return config::list_installed_bundles(); } - install_builtins()?; + // Inspection flags are look-don't-touch readouts: skip the builtins + // bootstrap so a pristine config dir stays empty. On such a dir the + // list flags list nothing because nothing is installed, by design. + // Agent/role runs still bootstrap: they load builtin tool definitions. + let inspection_only = (info_flag || cli.list_secrets || cli.mcp_list) + && cli.agent.is_none() + && cli.role.is_none(); + if !inspection_only { + install_builtins()?; + } if let Some(category) = cli.install_builtins { return config::install_assets(category); @@ -226,7 +235,15 @@ async fn main() -> Result<()> { cli.mcp_list || cli.mcp_get.is_some() || cli.mcp_remove.is_some() || cli.mcp_add.is_some(); if mcp_action { let cfg = Config::load_with_interpolation(true).await?; - let app_config = AppConfig::from_config(cfg)?; + let app_config = if cli.mcp_list + && cli.mcp_get.is_none() + && cli.mcp_remove.is_none() + && cli.mcp_add.is_none() + { + AppConfig::from_config_lenient(cfg)? + } else { + AppConfig::from_config(cfg)? + }; let vault = Vault::init(&app_config)?; mcp::manage::handle(&cli, &vault)?; @@ -236,7 +253,16 @@ async fn main() -> Result<()> { if vault_flags { let cfg = Config::load_with_interpolation(true).await?; - let app_config = AppConfig::from_config(cfg)?; + let app_config = if cli.list_secrets + && cli.add_secret.is_none() + && cli.get_secret.is_none() + && cli.update_secret.is_none() + && cli.delete_secret.is_none() + { + AppConfig::from_config_lenient(cfg)? + } else { + AppConfig::from_config(cfg)? + }; let vault = Vault::init(&app_config)?; return Vault::handle_vault_flags(cli, &vault); } @@ -244,7 +270,11 @@ async fn main() -> Result<()> { let abort_signal = create_abort_signal(); let start_mcp_servers = cli.agent.is_none() && cli.role.is_none(); let cfg = Config::load_with_interpolation(info_flag).await?; - let mut app_config = AppConfig::from_config(cfg)?; + let mut app_config = if info_flag { + AppConfig::from_config_lenient(cfg)? + } else { + AppConfig::from_config(cfg)? + }; if cli.no_workspace_mcp { app_config.no_workspace_mcp = true; } @@ -258,14 +288,20 @@ async fn main() -> Result<()> { log_path, start_mcp_servers, info_flag, + inspection_only, abort_signal.clone(), ) .await?, ); let mut ctx = RequestContext::bootstrap(app_state, working_mode, info_flag)?; let app_config = Arc::clone(&ctx.app.config); - ctx.bootstrap_tools(&app_config, start_mcp_servers, abort_signal.clone()) - .await?; + // Rebuilding the tool scope prunes and builds function binaries; + // inspection-only runs are readouts that never execute tools, so skip + // it there too. + if !inspection_only { + ctx.bootstrap_tools(&app_config, start_mcp_servers, abort_signal.clone()) + .await?; + } { let app = &*ctx.app.config; @@ -306,9 +342,12 @@ async fn run( } if cli.list_models { - for model in list_models(ctx.app.config.as_ref(), ModelType::Chat) { - println!("{}", model.id()); - } + let models = list_models(ctx.app.config.as_ref(), ModelType::Chat) + .iter() + .map(|model| model.id()) + .collect::>() + .join("\n"); + println!("{models}"); return Ok(()); } if cli.list_roles { diff --git a/src/testing.rs b/src/testing.rs index 4442565..380e7cd 100644 --- a/src/testing.rs +++ b/src/testing.rs @@ -9,25 +9,30 @@ struct TestLogCollector; static WARN_MESSAGES: OnceLock>> = OnceLock::new(); static DEBUG_MESSAGES: OnceLock>> = OnceLock::new(); -/// Debug capture is restricted to the hooks module so the buffer is not -/// flooded by debug output from the rest of the crate. Matching is -/// module-boundary-aware: only this exact module or its `::` submodules +/// Debug capture is restricted to the modules whose tests assert on debug +/// output so the buffer is not flooded by the rest of the crate. Matching is +/// module-boundary-aware: only these exact modules or their `::` submodules /// qualify, so a sibling like `hooks_registry` would not. -const DEBUG_TARGET_PREFIX: &str = concat!(env!("CARGO_CRATE_NAME"), "::hooks"); +const DEBUG_TARGET_PREFIXES: [&str; 2] = [ + concat!(env!("CARGO_CRATE_NAME"), "::hooks"), + concat!(env!("CARGO_CRATE_NAME"), "::config::agent"), +]; fn captures_warn(metadata: &Metadata) -> bool { metadata.level() <= Level::Warn } -fn captures_hooks_debug(metadata: &Metadata) -> bool { +fn captures_module_debug(metadata: &Metadata) -> bool { if metadata.level() != Level::Debug { return false; } let target = metadata.target(); - target == DEBUG_TARGET_PREFIX - || target - .strip_prefix(DEBUG_TARGET_PREFIX) - .is_some_and(|rest| rest.starts_with("::")) + DEBUG_TARGET_PREFIXES.iter().any(|prefix| { + target == *prefix + || target + .strip_prefix(prefix) + .is_some_and(|rest| rest.starts_with("::")) + }) } /// Every warn- or error-level message captured since the collector was @@ -38,9 +43,9 @@ pub(crate) fn warn_messages() -> &'static Mutex> { WARN_MESSAGES.get_or_init(Mutex::default) } -/// Every debug-level message from the hooks module captured since the -/// collector was installed. Same marker-filtering discipline as -/// [`warn_messages`]. +/// Every debug-level message from the [`DEBUG_TARGET_PREFIXES`] modules +/// captured since the collector was installed. Same marker-filtering +/// discipline as [`warn_messages`]. pub(crate) fn debug_messages() -> &'static Mutex> { DEBUG_MESSAGES.get_or_init(Mutex::default) } @@ -66,7 +71,7 @@ fn snapshot_of(buffer: &Mutex>) -> Vec { impl Log for TestLogCollector { fn enabled(&self, metadata: &Metadata) -> bool { - captures_warn(metadata) || captures_hooks_debug(metadata) + captures_warn(metadata) || captures_module_debug(metadata) } // A test that panics while holding a buffer poisons its mutex; the logger @@ -77,7 +82,7 @@ impl Log for TestLogCollector { .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()) .push(record.args().to_string()); - } else if captures_hooks_debug(record.metadata()) { + } else if captures_module_debug(record.metadata()) { debug_messages() .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()) @@ -89,10 +94,11 @@ impl Log for TestLogCollector { } /// Installs the process-wide log collector capturing warn-level messages and -/// hooks-module debug messages. `log::set_logger` accepts one logger per -/// process, so every test that captures either stream must install through -/// this shared entry point. The max level is Debug so the hooks debug capture -/// sees its records; warn capture is unaffected. +/// debug messages from the [`DEBUG_TARGET_PREFIXES`] modules. +/// `log::set_logger` accepts one logger per process, so every test that +/// captures either stream must install through this shared entry point. The +/// max level is Debug so the debug capture sees its records; warn capture is +/// unaffected. pub(crate) fn install_log_collector() { static INSTALL: Once = Once::new(); INSTALL.call_once(|| { diff --git a/src/vault/mod.rs b/src/vault/mod.rs index 43dad8a..4d187f2 100644 --- a/src/vault/mod.rs +++ b/src/vault/mod.rs @@ -332,7 +332,15 @@ impl Vault { } if cli.list_secrets { - vault.list_secrets(true)?; + // Listing is an inspection readout: report sandbox unavailability + // instead of failing like the mutating vault flags do. + if vault.sandbox_mode { + println!( + "The vault is unavailable in sandbox mode. Use `coyote --list-secrets` on your host." + ); + } else { + vault.list_secrets(true)?; + } } Ok(()) @@ -410,4 +418,23 @@ mod tests { } } } + + #[test] + fn list_secrets_flag_ok_in_sandbox() { + use clap::Parser; + + // Built directly rather than via IS_SANDBOX so this test cannot race + // the env juggling in vault_disabled_in_sandbox. + let vault = Vault { + sandbox_mode: true, + ..Vault::default() + }; + let cli = Cli::try_parse_from(["coyote", "--list-secrets"]).unwrap(); + + // Pins the --list-secrets sandbox contract: rc=0 with an informational + // readout instead of the hard failure the mutating flags keep. The + // message goes straight to stdout, so only Ok is asserted here. + Vault::handle_vault_flags(cli, &vault) + .expect("--list-secrets must succeed in sandbox mode"); + } } diff --git a/tests/inspection_flags.rs b/tests/inspection_flags.rs new file mode 100644 index 0000000..1486c23 --- /dev/null +++ b/tests/inspection_flags.rs @@ -0,0 +1,124 @@ +//! Binary-level contract for inspection flags on a pristine config dir: +//! exit 0 and NOTHING written into the config dir (no first-run wizard, +//! no builtins bootstrap). Only --info guarantees output on stdout; the +//! list flags legitimately emit an empty listing on a pristine dir, so +//! their contract is exit 0 + zero writes. Vault-backed flags like +//! --list-secrets depend on host state and are covered by +//! scripts/usage-probe-inspection.sh instead. Agent runs, by contrast, +//! must still bootstrap builtins even when combined with --info — see +//! agent_info_on_empty_config_dir_bootstraps_builtins. + +use std::env; +use std::fs; +use std::path::PathBuf; +use std::process::{Command, Stdio}; +use std::time::{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-inspection-{label}-{unique}")); + fs::create_dir_all(&tmp_dir).unwrap(); + tmp_dir +} + +fn probe_inspection_flag(flag: &str, require_stdout: bool) { + let tmp_dir = fresh_config_dir(flag.trim_start_matches("--")); + + let output = Command::new(env!("CARGO_BIN_EXE_coyote")) + .arg(flag) + .env("COYOTE_CONFIG_DIR", &tmp_dir) + .stdin(Stdio::null()) + .output() + .unwrap(); + + let leftover: Vec<_> = fs::read_dir(&tmp_dir) + .unwrap() + .map(|entry| entry.unwrap().file_name()) + .collect(); + let _ = fs::remove_dir_all(&tmp_dir); + + assert!( + output.status.success(), + "{flag}: expected exit 0 on an empty config dir, got {:?}\nstdout: {}\nstderr: {}", + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + if require_stdout { + assert!( + !output.stdout.is_empty(), + "{flag}: expected some output on stdout" + ); + } + assert!( + leftover.is_empty(), + "{flag}: inspection flags must write nothing into the config dir: {leftover:?}" + ); +} + +#[test] +fn info_on_empty_config_dir_writes_nothing() { + probe_inspection_flag("--info", true); +} + +// List flags on a pristine dir correctly print an empty listing, so the +// contract is exit 0 + zero writes only (no stdout assertion). + +#[test] +fn list_models_on_empty_config_dir_writes_nothing() { + probe_inspection_flag("--list-models", false); +} + +#[test] +fn list_agents_on_empty_config_dir_writes_nothing() { + probe_inspection_flag("--list-agents", false); +} + +#[test] +fn mcp_list_on_empty_config_dir_writes_nothing() { + probe_inspection_flag("--mcp-list", false); +} + +// The wizard bypass must not swallow the builtins bootstrap for agent runs: +// `--agent --info` on a totally fresh config dir has to install +// the bundled definitions before it can render the agent readout. +#[test] +fn agent_info_on_empty_config_dir_bootstraps_builtins() { + let tmp_dir = fresh_config_dir("agent-info"); + + let output = Command::new(env!("CARGO_BIN_EXE_coyote")) + .args(["--agent", "adversary", "--info"]) + .env("COYOTE_CONFIG_DIR", &tmp_dir) + .stdin(Stdio::null()) + .output() + .unwrap(); + + let agents_dir = tmp_dir.join("agents"); + let builtins_installed = + agents_dir.is_dir() && fs::read_dir(&agents_dir).unwrap().next().is_some(); + let _ = fs::remove_dir_all(&tmp_dir); + + assert!( + output.status.success(), + "--agent adversary --info: expected exit 0 on an empty config dir, got {:?}\nstdout: {}\nstderr: {}", + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!( + builtins_installed, + "--agent adversary --info: expected builtin agents installed into {}", + agents_dir.display() + ); + // The --info readout echoes the loaded agent definition including its + // global_tools, so a builtin tool name there proves the definitions were + // loaded into the agent's tool scope, not just written to disk. + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("fs_cat.sh"), + "--agent adversary --info: expected builtin tool fs_cat.sh in the readout\nstdout: {stdout}" + ); +}