fix(TASK-046): keep inspection leniency off mutating and writing flags
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).
This commit is contained in:
6 files changed
+269
-32
No files matched your search
@@ -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 </dev/null >"$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
|
||||
|
||||
Executable
+68
@@ -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"
|
||||
@@ -50,13 +50,15 @@ pub(super) fn model_completer(current: &OsStr) -> Vec<CompletionCandidate> {
|
||||
|
||||
fn load_app_config_for_completion() -> anyhow::Result<AppConfig> {
|
||||
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)
|
||||
|
||||
+10
-7
@@ -729,14 +729,17 @@ pub fn maybe_spawn_models_refresh(app: &AppConfig) {
|
||||
}
|
||||
|
||||
impl Config {
|
||||
pub async fn load_with_interpolation(info_flag: bool) -> Result<Self> {
|
||||
pub async fn load_with_interpolation(
|
||||
skip_interpolation: bool,
|
||||
inspection: bool,
|
||||
) -> Result<Self> {
|
||||
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);
|
||||
|
||||
+30
-20
@@ -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)?
|
||||
|
||||
+132
-1
@@ -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"
|
||||
);
|
||||
}
|
||||
Reference in new issue
Block a user