From 443d9dadff411827e22803ca7dffdb434a6ed1a4 Mon Sep 17 00:00:00 2001 From: BootIntel Agent Date: Sun, 27 Sep 2026 12:05:57 +0000 Subject: [PATCH] Release 0.6.1: the gate survives a pipe, and history is opt-in scan --gate-critical could report success on a capture that trips the gate. Measured on the 0.6.0 release binary, piping into head with output over the 64 KB pipe buffer: exit 1, then 0, then 0, on a log with telnet exposed. A CI job piping through head or tee would go green on a device with a critical exposure, non-deterministically, which is the worst direction for a gate to fail. Cause was ordering. The output write ran before the gate check, so a broken pipe propagated up and main.rs turned it into exit 0 before any verdict was evaluated. That handler is right that a reader closing the pipe is not an error, but it must not become a verdict. The pipe error is now swallowed at the write site only, and the gates decide on findings alone, which do not depend on whether anyone was still reading. Any other write error is still fatal, and manpage | head still exits 0 silently. Scan history is now opt in. It was on by default with an opt-out and a one-time notice, which is the wrong default for a tool whose pitch is that it uploads nothing: a record of every log path a consultant analysed should not appear on disk because nobody said no. `bootintel doctor` disclosing it on a fresh machine is what prompted this. no_history and BOOTINTEL_NO_HISTORY still mean off, so anyone who had opted out stays opted out, and an explicit off beats an explicit on so a machine-wide opt-out cannot be re-enabled by a config file. Numbered 0.6.1 by request. I flagged that a changed default is not really PATCH under the policy at the top of CHANGELOG.md, and that someone upgrading will find history silently stopped. The warning is therefore the first thing in the 0.6.1 entry rather than implied by a version digit. Two things the testing changed. My first attempt to reproduce the pipe bug used the default 361-byte output, well under the pipe buffer, so both binaries returned 1 and it looked unreproducible; inflating with --context exposed it. That is the second time today a negative result was an artefact of too small a test. And four tests failed when the history default inverted, correctly, and were updated to assert the new contract rather than weakened. Verified on the built binary: four consecutive piped gate runs all exit 1; manpage | head exits 0; no history file by default; one when BOOTINTEL_HISTORY=1; `config list` shows the new key. 310 tests pass, clippy clean under -D warnings, rustfmt clean, cargo deny all ok. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 46 +++++++++++- Cargo.lock | 4 +- Cargo.toml | 2 +- crates/cli/Cargo.toml | 2 +- crates/cli/src/cmd/scan/mod.rs | 34 ++++++++- crates/cli/src/config.rs | 131 +++++++++++++++++++++++++++++---- crates/cli/src/history.rs | 27 ++++--- 7 files changed, 212 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99c9fc7..d16d971 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,49 @@ All notable changes to bootintel-cli are documented here. Format follows [Keep a ## [Unreleased] +## [0.6.1] — 2026-09-27 — the gate survives a pipe, and history is opt-in + +**Read the second item before upgrading.** It changes a default, which the +policy above does not really call PATCH. It is numbered 0.6.1 by request; the +warning is here rather than in the version digit. + +### Fixed +- **`scan --gate-critical` could report success on a capture that trips the + gate.** Measured on the 0.6.0 release binary, piping into `head` with output + over the 64 KB pipe buffer: three runs gave exit 1, then 0, then 0, on a log + with telnet exposed. A CI job piping through `head` or `tee` would go green + on a device with a critical exposure, non-deterministically, which is the + worst direction for a gate to fail. + + Cause was ordering. The output write ran before the gate check, so a broken + pipe propagated up and the handler in main.rs turned it into exit 0 before + any verdict was evaluated. That handler is right that a reader closing the + pipe is not an error, but it must not become a verdict. The pipe error is now + swallowed at the write site only, and the gates decide on findings alone, + which do not depend on whether anyone was still reading. Any other write + error is still fatal, and `manpage | head` still exits 0 silently. + +### Changed +- **Scan history is now OPT IN. If you relied on it, it has stopped.** Enable + with `bootintel config set history true` or `BOOTINTEL_HISTORY=1`. + + It was on by default with an opt-out and a one-time notice. That is the wrong + default for a tool whose pitch is that it uploads nothing: a record of every + log path a consultant analysed should not appear on disk because nobody said + no. `bootintel doctor` disclosing it on a fresh machine is what prompted the + change. + + `no_history` and `BOOTINTEL_NO_HISTORY` still work and still mean off, so + anyone who had opted out stays opted out. An explicit off beats an explicit + on, so a machine-wide opt-out cannot be re-enabled by a config file. + +- Every "get an API key" message pointed at `bootintel.com/settings/api-keys`, + a route that has never existed; there is no `/settings` on the site. Keys + live at `/dashboard/developer`. Six places said otherwise: `doctor`, + `whoami` twice, `scan --api` twice, `analyze --api`. They now recommend + `bootintel login` first, which is the path for a person at a terminal and the + only one that works below Pro. + ### Fixed - **Every "get an API key" message pointed at a URL that 404s.** `https://bootintel.com/settings/api-keys` does not exist and never has; @@ -528,7 +571,8 @@ Initial release. All six subcommands live; five branch-based milestones (M1-M5) - PDF report download subcommand — server-side endpoint exists but no client-side wrapper yet. - Windows support — the Rust code compiles for Windows and the release workflow builds it, but install.sh doesn't handle Windows yet (`.ps1` installer is a follow-up). -[Unreleased]: https://github.com/bootintel/cli/compare/cli-v0.6.0...HEAD +[Unreleased]: https://github.com/bootintel/cli/compare/cli-v0.6.1...HEAD +[0.6.1]: https://github.com/bootintel/cli/releases/tag/cli-v0.6.1 [0.6.0]: https://github.com/bootintel/cli/releases/tag/cli-v0.6.0 [0.5.0]: https://github.com/bootintel/cli/releases/tag/cli-v0.5.0 [0.4.2]: https://github.com/bootintel/cli/releases/tag/cli-v0.4.2 diff --git a/Cargo.lock b/Cargo.lock index 4483efd..1f02def 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -131,7 +131,7 @@ checksum = "b588b76d00fde79687d7646a9b5bdf3cc0f655e0bbd080335a95d7e96f3587da" [[package]] name = "bootintel" -version = "0.6.0" +version = "0.6.1" dependencies = [ "anyhow", "arboard", @@ -154,7 +154,7 @@ dependencies = [ [[package]] name = "bootintel-detectors" -version = "0.6.0" +version = "0.6.1" dependencies = [ "regex", ] diff --git a/Cargo.toml b/Cargo.toml index 1a33644..d492ec2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,7 +19,7 @@ members = ["crates/detectors", "crates/cli"] resolver = "2" [workspace.package] -version = "0.6.0" +version = "0.6.1" edition = "2021" rust-version = "1.90" license = "Apache-2.0" diff --git a/crates/cli/Cargo.toml b/crates/cli/Cargo.toml index 2aef007..9ea073a 100644 --- a/crates/cli/Cargo.toml +++ b/crates/cli/Cargo.toml @@ -24,7 +24,7 @@ path = "src/main.rs" # version when packaging for crates.io, which rejects a bare path dep. # This is what publish = false was working around; publishing # bootintel-detectors first makes the workaround unnecessary. -bootintel-detectors = { path = "../detectors", version = "0.6.0" } +bootintel-detectors = { path = "../detectors", version = "0.6.1" } clap = { version = "4.5", features = ["derive", "wrap_help", "env"] } # Shell-completion emitters for `bootintel completions `. Version # tracks the clap major/minor (4.5). No default features — we only use diff --git a/crates/cli/src/cmd/scan/mod.rs b/crates/cli/src/cmd/scan/mod.rs index 9d93452..3f939a7 100644 --- a/crates/cli/src/cmd/scan/mod.rs +++ b/crates/cli/src/cmd/scan/mod.rs @@ -219,16 +219,42 @@ pub fn run(args: Args) -> Result<()> { let stdout = io::stdout(); let color = output::resolve_color_mode(args.no_color, &stdout); let mut out = stdout.lock(); - output::write( + // A closed pipe must not decide the gate. + // + // `bootintel scan --gate-critical | head` previously reported success on a + // capture with telnet exposed: output::write hit EPIPE, `?` propagated it, + // and the broken-pipe handler in main.rs turned that into exit 0 before + // any gate check ran. The reader having seen enough is not an error, which + // is why that handler exists, but it must not become a verdict. + // + // So a broken pipe is swallowed HERE, and only here. Every gate below + // decides on `findings` alone, which do not depend on whether anyone was + // still reading. Any other write error is still fatal. + let mut stdout_closed = false; + if let Err(e) = output::write( &mut out, &findings, args.format, &raw, color, &log.source_label, - )?; - if args.context > 0 && matches!(args.format, Format::Text) { - context::write_context_blocks(&mut out, &findings, &raw, args.context, color)?; + ) { + if crate::is_broken_pipe(&e) { + stdout_closed = true; + } else { + return Err(e); + } + } + if !stdout_closed && args.context > 0 && matches!(args.format, Format::Text) { + if let Err(e) = + context::write_context_blocks(&mut out, &findings, &raw, args.context, color) + { + // Nothing reads stdout_closed past this point, so there is + // nothing to record: stop writing and let the gates decide. + if !crate::is_broken_pipe(&e) { + return Err(e); + } + } } // Pre-compute the values we'll write to history, so the per-exit diff --git a/crates/cli/src/config.rs b/crates/cli/src/config.rs index 76b4e2c..5756388 100644 --- a/crates/cli/src/config.rs +++ b/crates/cli/src/config.rs @@ -46,6 +46,9 @@ pub struct Config { /// API key. Overridden by `$BOOTINTEL_API_KEY`. No CLI flag — /// keys aren't accepted on argv for shell-history reasons. pub api_key: Option, + /// Opt IN to the local scan history. Default false. + #[serde(default)] + pub history: bool, /// Default output format for `scan` / `batch` / `view`. Any /// `--format` flag on the command line takes precedence. pub default_format: Option, @@ -210,21 +213,49 @@ pub fn effective_default_format(cfg: &Config) -> Effective { /// Resolve the `no_history` boolean. Env var `BOOTINTEL_NO_HISTORY=1` /// wins; config `no_history = true` is the fallback. Any other env /// value ("0", "false", empty, unset) means "consult the file". +/// Whether scan history is DISABLED. Now true unless explicitly opted in. +/// +/// It used to be on by default with an opt-out, disclosed in a one-time notice. +/// That was the wrong default for a tool whose pitch is that it uploads +/// nothing: a record of every log path a consultant analysed is exactly the +/// kind of thing that should not appear on disk because nobody said no. +/// +/// `no_history` / BOOTINTEL_NO_HISTORY still work and still mean off, so +/// anyone who had opted out stays opted out. Enabling now takes +/// `history = true` or BOOTINTEL_HISTORY=1. pub fn effective_no_history(cfg: &Config) -> (bool, Source) { + // An explicit off wins over an explicit on, so a machine-wide opt-out + // cannot be silently re-enabled by a config file. match std::env::var("BOOTINTEL_NO_HISTORY").ok().as_deref() { Some("1") | Some("true") | Some("yes") => return (true, Source::Env), _ => {} } - // With `no_history: bool` (post-P2-9 unification), we can't tell - // "explicitly set to false" from "unset" — both are false. Report - // Source::File when true, Source::Default when false. In practice - // the callsites don't care about that distinction: `config list` - // shows a value + source, and false-from-file vs false-from-default - // behave identically at runtime. if cfg.no_history { - (true, Source::File) - } else { - (false, Source::Default) + return (true, Source::File); + } + match std::env::var("BOOTINTEL_HISTORY").ok().as_deref() { + Some("1") | Some("true") | Some("yes") => return (false, Source::Env), + Some("0") | Some("false") | Some("no") => return (true, Source::Env), + _ => {} + } + if cfg.history { + return (false, Source::File); + } + // Default: no history. + return (true, Source::Default); + #[allow(unreachable_code)] + { + // With `no_history: bool` (post-P2-9 unification), we can't tell + // "explicitly set to false" from "unset" — both are false. Report + // Source::File when true, Source::Default when false. In practice + // the callsites don't care about that distinction: `config list` + // shows a value + source, and false-from-file vs false-from-default + // behave identically at runtime. + if cfg.no_history { + (true, Source::File) + } else { + (false, Source::Default) + } } } @@ -309,6 +340,15 @@ pub fn get_effective(cfg: &Config, key: ConfigKey) -> Effective { ConfigKey::ApiBase => effective_api_base(cfg), ConfigKey::ApiKey => effective_api_key(cfg), ConfigKey::DefaultFormat => effective_default_format(cfg), + ConfigKey::History => { + // Reported as the positive, which is what the user sets, while + // effective_no_history stays the single source of truth. + let (disabled, src) = effective_no_history(cfg); + Effective { + value: Some((!disabled).to_string()), + source: src, + } + } ConfigKey::NoHistory => { let (v, src) = effective_no_history(cfg); Effective { @@ -325,7 +365,7 @@ pub fn get_effective(cfg: &Config, key: ConfigKey) -> Effective { pub fn get_key(cfg: &Config, key: &str) -> Result { let k = ConfigKey::from_str(key).ok_or_else(|| { anyhow::anyhow!( - "unknown config key '{}' — valid: api_base, api_key, default_format, no_history", + "unknown config key '{}' — valid: api_base, api_key, default_format, history, no_history", key ) })?; @@ -343,6 +383,7 @@ pub enum ConfigKey { ApiBase, ApiKey, DefaultFormat, + History, NoHistory, } @@ -354,6 +395,7 @@ impl ConfigKey { ConfigKey::ApiBase => "api_base", ConfigKey::ApiKey => "api_key", ConfigKey::DefaultFormat => "default_format", + ConfigKey::History => "history", ConfigKey::NoHistory => "no_history", } } @@ -366,6 +408,7 @@ impl ConfigKey { "api_base" => Some(ConfigKey::ApiBase), "api_key" => Some(ConfigKey::ApiKey), "default_format" => Some(ConfigKey::DefaultFormat), + "history" => Some(ConfigKey::History), "no_history" => Some(ConfigKey::NoHistory), _ => None, } @@ -379,6 +422,7 @@ pub const ALL_KEYS: &[ConfigKey] = &[ ConfigKey::ApiBase, ConfigKey::ApiKey, ConfigKey::DefaultFormat, + ConfigKey::History, ConfigKey::NoHistory, ]; @@ -413,7 +457,7 @@ pub fn resolve_api_key() -> Option { fn apply_key(cfg: &mut Config, key: &str, value: &str) -> Result<()> { let k = ConfigKey::from_str(key).ok_or_else(|| { anyhow::anyhow!( - "unknown config key '{}' — valid: api_base, api_key, default_format, no_history", + "unknown config key '{}' — valid: api_base, api_key, default_format, history, no_history", key ) })?; @@ -428,6 +472,15 @@ fn apply(cfg: &mut Config, key: ConfigKey, value: &str) -> Result<()> { ConfigKey::ApiBase => cfg.api_base = Some(value.to_string()), ConfigKey::ApiKey => cfg.api_key = Some(value.to_string()), ConfigKey::DefaultFormat => cfg.default_format = Some(value.to_string()), + ConfigKey::History => { + cfg.history = parse_bool(value) + .with_context(|| format!("history must be a boolean (true/false), got: {value}"))?; + // Setting history=true clears a stale opt-out, otherwise the + // explicit-off precedence would silently ignore the request. + if cfg.history { + cfg.no_history = false; + } + } ConfigKey::NoHistory => { cfg.no_history = parse_bool(value).with_context(|| { format!("no_history must be a boolean (true/false), got: {value}") @@ -475,6 +528,7 @@ mod tests { std::env::remove_var("BOOTINTEL_API_KEY"); std::env::remove_var("BOOTINTEL_API_BASE"); std::env::remove_var("BOOTINTEL_NO_HISTORY"); + std::env::remove_var("BOOTINTEL_HISTORY"); } #[test] @@ -568,16 +622,65 @@ mod tests { assert_eq!(src, Source::File); } + /// The default INVERTED: history is off unless asked for. It used to be on + /// with an opt-out, which is the wrong default for a tool whose pitch is + /// that it uploads nothing. A record of every log path a consultant + /// analysed should not appear on disk because nobody said no. #[test] - fn no_history_default_false() { + fn history_is_off_by_default() { let _g = env_lock(); clear_env(); let cfg = Config::default(); - let (v, src) = effective_no_history(&cfg); - assert!(!v); + let (disabled, src) = effective_no_history(&cfg); + assert!(disabled, "history must be off unless opted in"); assert_eq!(src, Source::Default); } + #[test] + fn history_can_be_opted_into_by_file_or_env() { + let _g = env_lock(); + clear_env(); + let cfg = Config { + history: true, + ..Default::default() + }; + assert!( + !effective_no_history(&cfg).0, + "history=true should enable it" + ); + + clear_env(); + std::env::set_var("BOOTINTEL_HISTORY", "1"); + assert!( + !effective_no_history(&Config::default()).0, + "env should enable it" + ); + clear_env(); + } + + /// An explicit off beats an explicit on, so a machine-wide opt-out cannot + /// be silently re-enabled by a config file that happens to say history. + #[test] + fn an_explicit_opt_out_wins_over_an_opt_in() { + let _g = env_lock(); + clear_env(); + let cfg = Config { + history: true, + no_history: true, + ..Default::default() + }; + assert!(effective_no_history(&cfg).0); + + clear_env(); + std::env::set_var("BOOTINTEL_NO_HISTORY", "1"); + let cfg2 = Config { + history: true, + ..Default::default() + }; + assert!(effective_no_history(&cfg2).0); + clear_env(); + } + #[test] fn set_roundtrip_via_atomic_write() { // We can't easily point set_key at a temp dir without diff --git a/crates/cli/src/history.rs b/crates/cli/src/history.rs index dd461e6..9cfde6a 100644 --- a/crates/cli/src/history.rs +++ b/crates/cli/src/history.rs @@ -115,7 +115,7 @@ pub fn is_disabled() -> bool { fn try_append(entry: &Entry) -> Result<()> { if is_disabled() { - crate::vdebug!("history: writes disabled (BOOTINTEL_NO_HISTORY / no_history=true)"); + crate::vdebug!("history: off (default). Enable with `bootintel config set history true` or BOOTINTEL_HISTORY=1"); return Ok(()); } let path = history_path().context("no platform state dir resolvable")?; @@ -161,25 +161,24 @@ fn try_append(entry: &Entry) -> Result<()> { Ok(()) } -/// Tell the user, once, that we are keeping a local scan history. +/// Confirm, once, that the history the user asked for has started. /// -/// Called only when the history file is about to be created for the -/// first time. stderr, so it never lands in a redirected report. +/// This used to disclose a history that was on by default and offer a way +/// out. It is now opt-in, so the notice confirms a choice instead of +/// announcing a surprise, and no longer needs to explain how to escape. +/// Called only when the file is about to be created. stderr, so it never +/// lands in a redirected report. fn announce_first_write(path: &std::path::Path) { if crate::verbose::is_quiet() { return; } eprintln!( - "[bootintel] note: recording a local scan history at {}", + "[bootintel] scan history enabled; recording to {}", path.display() ); eprintln!("[bootintel] One line per scan: timestamp, log path, finding counts, exit code."); - eprintln!("[bootintel] It stays on this machine — bootintel uploads nothing without --api."); - eprintln!("[bootintel] Turn it off with `bootintel config set no_history true`, or"); - eprintln!( - "[bootintel] per-run with BOOTINTEL_NO_HISTORY=1. Inspect it with `bootintel history`." - ); - eprintln!("[bootintel] This notice prints once."); + eprintln!("[bootintel] It stays on this machine. Inspect it with `bootintel history`."); + eprintln!("[bootintel] Stop with `bootintel config set history false`. Prints once."); } /// Open the history file for append. On Unix we use `.mode(0o600)` @@ -402,6 +401,8 @@ mod tests { std::env::set_var("XDG_STATE_HOME", &td); // Make sure no_history isn't set anywhere in the env. std::env::remove_var("BOOTINTEL_NO_HISTORY"); + // History is opt-in now, so clearing the opt-out is no longer enough. + std::env::set_var("BOOTINTEL_HISTORY", "1"); for i in 0..5 { append_entry(&Entry { @@ -480,6 +481,8 @@ mod tests { let saved = std::env::var("XDG_STATE_HOME").ok(); std::env::set_var("XDG_STATE_HOME", &td); std::env::remove_var("BOOTINTEL_NO_HISTORY"); + // History is opt-in now, so clearing the opt-out is no longer enough. + std::env::set_var("BOOTINTEL_HISTORY", "1"); append_entry(&Entry { ts: "2026-08-29T00:00:00Z".into(), @@ -520,6 +523,8 @@ mod tests { let saved = std::env::var("XDG_STATE_HOME").ok(); std::env::set_var("XDG_STATE_HOME", &td); std::env::remove_var("BOOTINTEL_NO_HISTORY"); + // History is opt-in now, so clearing the opt-out is no longer enough. + std::env::set_var("BOOTINTEL_HISTORY", "1"); let path = history_path().unwrap(); std::fs::create_dir_all(path.parent().unwrap()).unwrap();