From d89d24aff4783dcf6d123771e4d53a114e08bf8e Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Wed, 16 Sep 2026 16:48:57 -0600 Subject: [PATCH] fix(TASK-046): keep inspection leniency off mutating and writing flags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The look-don't-touch bypass was keyed on load_with_interpolation's pre-existing bool, which mutating early paths (--authenticate, --auth-mcp, mcp manage, vault flags) already passed as true meaning only 'skip vault interpolation' — so they silently lost their first-run config creation. Split the parameter: skip_interpolation keeps the historical meaning, a distinct inspection flag alone gates the missing-config default/wizard bypass. inspection_only is now computed from an explicit read-only whitelist: --sync-models is excluded (it downloads and writes models-override.yaml) and returns to the bootstrap/strict path, keeping only its historical interpolation-skip and vault leniency. mcp/vault inspection sub-conditions are hoisted and shared so mutating flag combos never inherit the bypass. Coverage: sandbox vault contract tests, a sync-models bootstrap-path pin, a --sync-models scenario in usage-probe-inspection.sh, and a PTY wizard regression probe (scripts/usage-probe-wizard.sh). --- scripts/usage-probe-inspection.sh | 23 ++++++ scripts/usage-probe-wizard.sh | 68 +++++++++++++++ src/cli/completer.rs | 10 ++- src/config/mod.rs | 17 ++-- src/main.rs | 50 ++++++----- tests/inspection_flags.rs | 133 +++++++++++++++++++++++++++++- 6 files changed, 269 insertions(+), 32 deletions(-) create mode 100755 scripts/usage-probe-wizard.sh diff --git a/scripts/usage-probe-inspection.sh b/scripts/usage-probe-inspection.sh index 5be12cb..b7e5543 100755 --- a/scripts/usage-probe-inspection.sh +++ b/scripts/usage-probe-inspection.sh @@ -82,6 +82,29 @@ probe_flag allow-empty --list-bundles probe_flag allow-empty --list-secrets probe_flag allow-empty --mcp-list +# --sync-models writes models-override.yaml, so it must NOT behave like an +# inspection flag: on a fresh empty config dir it takes the bootstrap path +# (builtins written) and, off a terminal, fails on the missing config. +# IS_SANDBOX is stripped: the host path is the one under test, and a leaked +# sandbox flag would reroute the missing config to the wizard prompt. +echo "== --sync-models on a pristine COYOTE_CONFIG_DIR: bootstrap path, strict failure ==" +sync_cfg="$(mktemp -d "$WORKDIR/cfg.XXXXXX")" +sync_out="$WORKDIR/sync-out.txt" +sync_rc=0 +env -u IS_SANDBOX -u COYOTE_PROVIDER -u COYOTE_PLATFORM COYOTE_CONFIG_DIR="$sync_cfg" \ + timeout 30 "$BIN" --sync-models "$sync_out" 2>&1 || sync_rc=$? +if [ "$sync_rc" = "124" ]; then + fail "--sync-models: hung (likely waiting on a prompt) on an empty config dir" +else + if [ "$sync_rc" = "0" ]; then + fail "--sync-models: expected the strict missing-config failure on an empty config dir, got rc=0" + fi + sync_nwritten="$(find "$sync_cfg" -mindepth 1 | wc -l | tr -d ' ')" + if [ "$sync_nwritten" = "0" ]; then + fail "--sync-models: expected the builtins bootstrap to populate the empty config dir, found nothing" + fi +fi + if [ "$failures" != "0" ]; then echo echo "$failures inspection-flag scenario(s) FAILED" >&2 diff --git a/scripts/usage-probe-wizard.sh b/scripts/usage-probe-wizard.sh new file mode 100755 index 0000000..947c5d5 --- /dev/null +++ b/scripts/usage-probe-wizard.sh @@ -0,0 +1,68 @@ +#!/usr/bin/env bash +# Black-box usage-pattern probe for the first-run wizard on a real PTY. +# +# Contract under test (companion to scripts/usage-probe-inspection.sh, which +# intentionally cannot assert this because it needs a pty): +# Plain `coyote` (no flags, no message) on a TTY with an EMPTY +# COYOTE_CONFIG_DIR must still reach the first-run wizard — the +# look-don't-touch bypass for inspection flags must NOT swallow the +# interactive setup path. We assert the wizard's first prompt +# ("No config file, create a new one?") appears within the timeout; the +# run is then killed, so the exit code is not asserted. +# +# Usage: scripts/usage-probe-wizard.sh +# Requires: a `coyote` binary on PATH or at ./target/debug/coyote or +# ./target/release/coyote, and the `script` utility (util-linux or BSD). + +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 + +if ! command -v script > /dev/null 2>&1; then + echo "SKIP: 'script' utility not available; cannot allocate a pty" >&2 + exit 0 +fi + +WORKDIR="$(mktemp -d)" +trap 'rm -rf "$WORKDIR"' EXIT + +CFG="$WORKDIR/cfg" +mkdir -p "$CFG" +OUT="$WORKDIR/out.txt" + +echo "== Plain \`coyote\` on a pty with an empty COYOTE_CONFIG_DIR reaches the wizard ==" + +# `script` allocates a real pty for the child, so IS_STDOUT_TERMINAL is true +# inside even in headless CI. IS_SANDBOX is stripped to exercise the plain +# host path from the recipe. The wizard blocks on its prompt forever, so a +# timeout kill (rc 124) is the expected way out. +rc=0 +if script --version 2>/dev/null | grep -q util-linux; then + env -u IS_SANDBOX COYOTE_CONFIG_DIR="$CFG" \ + timeout 15 script -qec "$BIN" /dev/null > "$OUT" 2>&1 < /dev/null || rc=$? +else + # BSD/macOS script: command goes after the typescript file. + env -u IS_SANDBOX COYOTE_CONFIG_DIR="$CFG" \ + timeout 15 script -q "$OUT" "$BIN" < /dev/null > /dev/null 2>&1 || rc=$? +fi + +if ! grep -aq "No config file, create a new one?" "$OUT"; then + echo "FAIL: wizard prompt never appeared (rc=$rc); output was:" >&2 + head -c 500 "$OUT" >&2 + exit 1 +fi + +echo "PASS: first-run wizard prompt appeared on a pty with an empty config dir" +echo +echo "ALL SCENARIOS PASSED" diff --git a/src/cli/completer.rs b/src/cli/completer.rs index b64667c..6e356f5 100644 --- a/src/cli/completer.rs +++ b/src/cli/completer.rs @@ -50,13 +50,15 @@ pub(super) fn model_completer(current: &OsStr) -> Vec { fn load_app_config_for_completion() -> anyhow::Result { let h = tokio::runtime::Handle::try_current().ok(); + // Tab completion is a read-only inspection surface: never run the + // first-run wizard or write anything from a completer. let cfg = match h { - Some(handle) => { - tokio::task::block_in_place(|| handle.block_on(Config::load_with_interpolation(true)))? - } + Some(handle) => tokio::task::block_in_place(|| { + handle.block_on(Config::load_with_interpolation(true, true)) + })?, None => { let rt = tokio::runtime::Runtime::new()?; - rt.block_on(Config::load_with_interpolation(true))? + rt.block_on(Config::load_with_interpolation(true, true))? } }; AppConfig::from_config(cfg) diff --git a/src/config/mod.rs b/src/config/mod.rs index 3ba4323..497c679 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -729,14 +729,17 @@ pub fn maybe_spawn_models_refresh(app: &AppConfig) { } impl Config { - pub async fn load_with_interpolation(info_flag: bool) -> Result { + pub async fn load_with_interpolation( + skip_interpolation: bool, + inspection: bool, + ) -> Result { let config_path = paths::config_file(); if env::var_os(SANDBOX_ENV_FLAG).is_some() { if !config_path.exists() { // Inspection-only flags are look-don't-touch: no first-run // config creation, nothing written. - if info_flag { + if inspection { return Ok(Self::default()); } create_config_file(&config_path).await?; @@ -756,7 +759,7 @@ impl Config { // Same look-don't-touch rule: never run the interactive // first-run wizard (or write a config) for a flag that // only inspects state. - if info_flag { + if inspection { return Ok(Self::default()); } if *IS_STDOUT_TERMINAL { @@ -769,7 +772,7 @@ impl Config { Self::load_from_file(&config_path)? }; - if info_flag { + if skip_interpolation { return Ok(config); } @@ -1627,7 +1630,7 @@ hooks: std::env::set_var(crate::sandbox::SANDBOX_ENV_FLAG, "1"); } - let result = Config::load_with_interpolation(false).await; + let result = Config::load_with_interpolation(false, false).await; let (_, raw) = Config::load_from_file(&config_path).unwrap(); unsafe { @@ -1666,7 +1669,7 @@ hooks: let _provider = crate::testing::EnvVarGuard::unset(get_env_name("provider")); let _platform = crate::testing::EnvVarGuard::unset(get_env_name("platform")); - let result = Config::load_with_interpolation(true).await; + let result = Config::load_with_interpolation(true, true).await; let leftover: Vec<_> = std::fs::read_dir(&tmp_dir) .unwrap() @@ -1694,7 +1697,7 @@ hooks: crate::testing::EnvVarGuard::set(get_env_name("config_file"), &config_path); let _sandbox = crate::testing::EnvVarGuard::set(crate::sandbox::SANDBOX_ENV_FLAG, "1"); - let result = Config::load_with_interpolation(true).await; + let result = Config::load_with_interpolation(true, true).await; let created = config_path.exists(); let _ = std::fs::remove_dir_all(&tmp_dir); diff --git a/src/main.rs b/src/main.rs index 012c511..39981e4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -113,6 +113,19 @@ async fn main() -> Result<()> { || cli.list_macros || cli.list_skills || cli.list_sessions; + // Read-only whitelist: --sync-models is excluded because it downloads + // and writes models-override.yaml; combined with a readout flag the run + // still writes (run() handles --sync-models first), so it disqualifies + // the whole invocation. + let readout_flag = (cli.info + || cli.list_models + || cli.list_roles + || cli.list_agents + || cli.list_rags + || cli.list_macros + || cli.list_skills + || cli.list_sessions) + && !cli.sync_models; let vault_flags = cli.add_secret.is_some() || cli.get_secret.is_some() || cli.update_secret.is_some() @@ -144,9 +157,15 @@ async fn main() -> Result<()> { // 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(); + let mcp_inspect = + cli.mcp_list && cli.mcp_get.is_none() && cli.mcp_remove.is_none() && cli.mcp_add.is_none(); + let vault_inspect = cli.list_secrets + && cli.add_secret.is_none() + && cli.get_secret.is_none() + && cli.update_secret.is_none() + && cli.delete_secret.is_none(); + let inspection_only = + (readout_flag || vault_inspect || mcp_inspect) && cli.agent.is_none() && cli.role.is_none(); if !inspection_only { install_builtins()?; } @@ -173,7 +192,7 @@ async fn main() -> Result<()> { } if let Some(client_arg) = &cli.authenticate { - let cfg = Config::load_with_interpolation(true).await?; + let cfg = Config::load_with_interpolation(true, false).await?; let app_config = AppConfig::from_config(cfg)?; let (client_name, provider) = resolve_oauth_client(client_arg.as_deref(), &app_config.clients)?; @@ -182,7 +201,7 @@ async fn main() -> Result<()> { } if let Some(server_name) = &cli.auth_mcp { - let cfg = Config::load_with_interpolation(true).await?; + let cfg = Config::load_with_interpolation(true, false).await?; let app_config = AppConfig::from_config(cfg)?; let vault = Vault::init(&app_config)?; let mcp_path = paths::mcp_config_file(); @@ -234,12 +253,8 @@ async fn main() -> Result<()> { let mcp_action = 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 = if cli.mcp_list - && cli.mcp_get.is_none() - && cli.mcp_remove.is_none() - && cli.mcp_add.is_none() - { + let cfg = Config::load_with_interpolation(true, mcp_inspect).await?; + let app_config = if mcp_inspect { AppConfig::from_config_lenient(cfg)? } else { AppConfig::from_config(cfg)? @@ -252,13 +267,8 @@ async fn main() -> Result<()> { } if vault_flags { - let cfg = Config::load_with_interpolation(true).await?; - 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() - { + let cfg = Config::load_with_interpolation(true, vault_inspect).await?; + let app_config = if vault_inspect { AppConfig::from_config_lenient(cfg)? } else { AppConfig::from_config(cfg)? @@ -269,8 +279,8 @@ 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 = if info_flag { + let cfg = Config::load_with_interpolation(info_flag, readout_flag).await?; + let mut app_config = if readout_flag { AppConfig::from_config_lenient(cfg)? } else { AppConfig::from_config(cfg)? diff --git a/tests/inspection_flags.rs b/tests/inspection_flags.rs index 1486c23..d96a293 100644 --- a/tests/inspection_flags.rs +++ b/tests/inspection_flags.rs @@ -6,7 +6,9 @@ //! --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. +//! agent_info_on_empty_config_dir_bootstraps_builtins. Flags that write, +//! like --sync-models, are NOT inspection flags and must stay on the +//! bootstrap path — see sync_models_stays_on_bootstrap_path. use std::env; use std::fs; @@ -122,3 +124,132 @@ fn agent_info_on_empty_config_dir_bootstraps_builtins() { "--agent adversary --info: expected builtin tool fs_cat.sh in the readout\nstdout: {stdout}" ); } + +// Sandbox-mode vault contract: `--list-secrets` is an inspection readout and +// must exit 0 with an informational message (and zero writes) when the vault +// is unreachable inside a sandbox, while mutating vault flags stay strict. +#[test] +fn sandbox_list_secrets_prints_informational_message_and_writes_nothing() { + let tmp_dir = fresh_config_dir("sandbox-list-secrets"); + + let output = Command::new(env!("CARGO_BIN_EXE_coyote")) + .arg("--list-secrets") + .env("COYOTE_CONFIG_DIR", &tmp_dir) + .env("IS_SANDBOX", "1") + .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(), + "--list-secrets in sandbox mode: expected exit 0, got {:?}\nstdout: {}\nstderr: {}", + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + let combined = format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!( + combined.contains("The vault is unavailable in sandbox mode"), + "--list-secrets in sandbox mode: expected the informational readout, got: {combined}" + ); + assert!( + leftover.is_empty(), + "--list-secrets in sandbox mode: expected zero writes into the config dir: {leftover:?}" + ); +} + +// Mutating vault flags are NOT inspection flags: in sandbox mode they must +// fail hard with the vault-management error, never the soft informational +// message. A minimal valid config is provided so the run gets past model +// resolution and reaches the vault gate. +#[test] +fn sandbox_mutating_vault_flag_stays_strict() { + let tmp_dir = fresh_config_dir("sandbox-delete-secret"); + fs::write( + tmp_dir.join("config.yaml"), + "model: dryrun:dry-model\n\ + dry_run: true\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", + ) + .unwrap(); + + let output = Command::new(env!("CARGO_BIN_EXE_coyote")) + .args(["--delete-secret", "probe"]) + .env("COYOTE_CONFIG_DIR", &tmp_dir) + .env("IS_SANDBOX", "1") + .stdin(Stdio::null()) + .output() + .unwrap(); + let _ = fs::remove_dir_all(&tmp_dir); + + assert!( + !output.status.success(), + "--delete-secret in sandbox mode: expected a non-zero exit, got {:?}\nstdout: {}", + output.status, + String::from_utf8_lossy(&output.stdout) + ); + let combined = format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!( + combined.contains("Vault management is disabled in sandbox mode"), + "--delete-secret in sandbox mode: expected the strict vault error, got: {combined}" + ); +} + +// --sync-models downloads and writes models-override.yaml, so it is NOT an +// inspection flag: on a pristine config dir it must take the bootstrap path +// (builtins installed) and, off a terminal, fail on the missing config +// instead of silently operating on defaults. The host path is the one under +// test: a leaked IS_SANDBOX (or provider env) would reroute the missing +// config to the sandbox wizard prompt instead of the fast failure. +#[test] +fn sync_models_stays_on_bootstrap_path() { + let tmp_dir = fresh_config_dir("sync-models"); + + let output = Command::new(env!("CARGO_BIN_EXE_coyote")) + .arg("--sync-models") + .env("COYOTE_CONFIG_DIR", &tmp_dir) + .env_remove("IS_SANDBOX") + .env_remove("COYOTE_PROVIDER") + .env_remove("COYOTE_PLATFORM") + .stdin(Stdio::null()) + .output() + .unwrap(); + + let bootstrapped = fs::read_dir(&tmp_dir).unwrap().next().is_some(); + let _ = fs::remove_dir_all(&tmp_dir); + + assert!( + !output.status.success(), + "--sync-models: expected the strict missing-config failure on an empty config dir, got {:?}\nstdout: {}\nstderr: {}", + output.status, + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert!( + bootstrapped, + "--sync-models: expected the builtins bootstrap to populate the config dir" + ); +}