diff --git a/crates/bitty-config/src/file.rs b/crates/bitty-config/src/file.rs index 3f9ec1a1c..df04fac48 100644 --- a/crates/bitty-config/src/file.rs +++ b/crates/bitty-config/src/file.rs @@ -567,16 +567,29 @@ impl CliOverrides { /// Validates the CTX-0180 appearance raws without needing layers. /// - /// Checks `--font-family` length and parses `--font-size`/`--opacity` - /// through the same typed validators the merge uses, so range violations - /// fail closed with the dotted field path. `theme` is intentionally NOT - /// checked here (CTX-0169 merge-time behavior stays untouched). + /// Checks `--font-family` length, parses `--font-size`/`--opacity` + /// through the same typed validators the merge uses, and validates + /// `--theme` against the preset registry, so every violation fails + /// closed with the dotted field path before any layer merge. + /// + /// CTX-1339: `theme` used to be checked only at merge time. Validating it + /// here keeps an invalid `--theme` attributable to the CLI layer even when + /// a valid user file is also present, so startup never treats the file as + /// the cause and never falls back to built-ins for a CLI usage error. /// /// # Errors /// - /// [`ConfigError::validation`] at `font.family` / `font.size` / - /// `window.opacity` for overlong/unparseable/out-of-range values. + /// [`ConfigError::validation`] at `appearance.theme` / `font.family` / + /// `font.size` / `window.opacity` for unknown or overlong theme names, + /// overlong/unparseable/out-of-range values. pub fn validate_appearance_overrides(&self) -> Result<(), ConfigError> { + if let Some(theme) = cli_present(self.theme.as_deref()) { + AppearanceConfig { + theme: Some(theme), + ..Default::default() + } + .validate()?; + } if let Some(family) = cli_present(self.font_family.as_deref()) { if family.len() > MAX_FONT_FAMILY_LEN { return Err(ConfigError::validation( @@ -5113,6 +5126,17 @@ mod tests { font_family: Some("x".repeat(MAX_FONT_FAMILY_LEN + 1)), ..Default::default() }, + // CTX-1339: `--theme` is pre-validated before merge, so an + // unknown or malformed name fails with its dotted field even + // when a valid user file also participates. + CliOverrides { + theme: Some("not-a-theme".to_string()), + ..Default::default() + }, + CliOverrides { + theme: Some("light:dracula".to_string()), + ..Default::default() + }, ] { assert!(!cli.is_empty()); let err = resolve_effective_full(Some(file_layer_with_appearance()), None, &cli) @@ -5121,7 +5145,8 @@ mod tests { assert!( msg.contains("font.size") || msg.contains("window.opacity") - || msg.contains("font.family"), + || msg.contains("font.family") + || msg.contains("appearance.theme"), "must name the field: {msg}" ); } diff --git a/crates/bitty-config/src/plan.rs b/crates/bitty-config/src/plan.rs index 856d80e54..1ef55136e 100644 --- a/crates/bitty-config/src/plan.rs +++ b/crates/bitty-config/src/plan.rs @@ -332,8 +332,8 @@ impl ConfigPlan { /// Distribution < Profile < User < TrustedLocal < Cli` #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] pub enum LayerKind { - /// Built-in minimal configuration; used for `--safe` and first-start - /// fallback per R-009. + /// Built-in minimal configuration; the first-start fallback per R-009 + /// uses it unmodified, while `--safe` applies safe-mode decoration on top. CoreDefaults, /// System defaults (e.g. `/etc/xdg/bitty/defaults.lua`). Trusted only /// after source verification. diff --git a/crates/bitty-terminal/src/config_cli.rs b/crates/bitty-terminal/src/config_cli.rs index 3c214192c..965ec2bd0 100644 --- a/crates/bitty-terminal/src/config_cli.rs +++ b/crates/bitty-terminal/src/config_cli.rs @@ -57,6 +57,71 @@ pub(crate) struct LoadedConfig { pub(crate) profile_path: Option, } +/// A config-load failure plus whether the first-start fallback may absorb it. +/// +/// Only a *present* user config file that cannot be loaded or merged is +/// recoverable (`broken`). Explicitly missing `--config`/`BITTY_CONFIG` paths, +/// missing or invalid profiles, CLI usage errors, and an invalid built-in +/// plan stay fail-closed (`fatal`), matching `lua-and-xdg.md` §Layers. +struct LoadFailure { + /// User-facing message; unchanged from the pre-fallback behavior. + message: String, + /// True only for a present-but-unusable external user file. + fallback_allowed: bool, +} + +impl LoadFailure { + /// A present external layer is unusable; first start may use built-ins. + fn broken(message: String) -> Self { + Self { + message, + fallback_allowed: true, + } + } + + /// Missing input, usage error, profile selection, or internal defect. + fn fatal(message: String) -> Self { + Self { + message, + fallback_allowed: false, + } + } + + fn into_message(self) -> String { + self.message + } +} + +/// Attributes a merge failure to the layer that actually caused it. +/// +/// `user_file_causal` is the outcome of re-merging without the user layer: +/// when that re-merge succeeds, the user file's contribution is what makes +/// the combined config invalid, so first start may fall back. When it still +/// fails, a profile or CLI layer owns the error and startup stays +/// fail-closed (CTX-1339 review finding: file presence alone is not causal). +fn classify_merge_failure( + err: &bitty_config::ConfigError, + user_path: Option<&std::path::Path>, + profile_path: Option<&std::path::Path>, + user_file_causal: bool, +) -> LoadFailure { + if user_file_causal { + if let Some(path) = user_path { + return LoadFailure::broken(format!( + "bitty: invalid config '{}': {err}", + path.display() + )); + } + } + if let Some(path) = profile_path { + return LoadFailure::fatal(format!( + "bitty: invalid profile '{}': {err}", + path.display() + )); + } + LoadFailure::fatal(format!("bitty: invalid --theme: {err}")) +} + /// Builds the single CLI override input from parsed [`Args`] (CTX-0180). /// /// Pure over `args` so the wiring stays hermetic: trims each raw and treats @@ -94,10 +159,11 @@ pub(crate) fn cli_overrides_from_args(args: &Args) -> bitty_config::file::CliOve } /// Names the CLI flag behind an appearance-override validation error for the -/// fail-closed message. `theme` keeps its CTX-0169 merge-time path and never -/// reaches here; anything unexpected falls back to the flag group. +/// fail-closed message. `theme` is pre-validated by CTX-1339 (it used to fail +/// only at merge); any unexpected field falls back to the flag group. pub(crate) fn appearance_flag_for_field(field: Option<&str>) -> &'static str { match field { + Some("appearance.theme") => "--theme", Some("font.family") => "--font-family", Some("font.size") => "--font-size", Some("window.opacity") => "--opacity", @@ -131,6 +197,11 @@ pub(crate) fn appearance_flag_for_field(field: Option<&str>) -> &'static str { /// profile identity. impure (filesystem + env); total (all failures become /// `Err(String)`). pub(crate) fn load_merged_config(args: &Args) -> Result { + resolve_merged_config(args).map_err(LoadFailure::into_message) +} + +/// Classified form of [`load_merged_config`] for the startup fallback. +fn resolve_merged_config(args: &Args) -> Result { // CTX-0346 `--safe` (R-009/P0-AC-019, spec rule 5): select the built-in // safe effective config and do not read any external layer. User files, // `BITTY_CONFIG`, profiles, and CLI appearance overrides are ignored — @@ -140,8 +211,9 @@ pub(crate) fn load_merged_config(args: &Args) -> Result { // user config can never abort safe startup or validation. Non-safe // behavior is unchanged. if args.safe { - let merged = bitty_config::safe_merged() - .map_err(|err| format!("bitty: invalid built-in safe config: {err}"))?; + let merged = bitty_config::safe_merged().map_err(|err| { + LoadFailure::fatal(format!("bitty: invalid built-in safe config: {err}")) + })?; return Ok(LoadedConfig { merged, probed: None, @@ -172,23 +244,23 @@ pub(crate) fn load_merged_config(args: &Args) -> Result { file_layer = Some(layer); } Err(err) => { - return Err(format!( + return Err(LoadFailure::broken(format!( "bitty: invalid config file '{}': {err}", probe.path.display() - )); + ))); } } } else if probe.explicit { if explicit_from_cli { - return Err(format!( + return Err(LoadFailure::fatal(format!( "bitty: --config '{}' not found", probe.path.display() - )); + ))); } - return Err(format!( + return Err(LoadFailure::fatal(format!( "bitty: BITTY_CONFIG '{}' not found", probe.path.display() - )); + ))); } } // Named profile (fail-closed): validate, resolve under the user-level @@ -206,22 +278,22 @@ pub(crate) fn load_merged_config(args: &Args) -> Result { if let Some(requested) = profile_request.clone() { let resolved = bitty_config::file::profile_file_path(&requested).map_err(|err| { if profile_from_cli { - format!("bitty: invalid --profile '{requested}': {err}") + LoadFailure::fatal(format!("bitty: invalid --profile '{requested}': {err}")) } else { - format!("bitty: invalid BITTY_PROFILE '{requested}': {err}") + LoadFailure::fatal(format!("bitty: invalid BITTY_PROFILE '{requested}': {err}")) } })?; if !resolved.exists() { if profile_from_cli { - return Err(format!( + return Err(LoadFailure::fatal(format!( "bitty: --profile '{requested}' not found ('{}')", resolved.display() - )); + ))); } - return Err(format!( + return Err(LoadFailure::fatal(format!( "bitty: BITTY_PROFILE '{requested}' not found ('{}')", resolved.display() - )); + ))); } // CTX-0759 (#1366): resolve the single-parent `extends` chain // (base-first `Profile` layers); the child overrides the base per @@ -233,10 +305,10 @@ pub(crate) fn load_merged_config(args: &Args) -> Result { profile_layers = layers; } Err(err) => { - return Err(format!( + return Err(LoadFailure::fatal(format!( "bitty: invalid profile file '{}': {err}", resolved.display() - )); + ))); } } // `--config` + `--profile` together: both layers stay loaded; the @@ -251,25 +323,40 @@ pub(crate) fn load_merged_config(args: &Args) -> Result { } // CTX-0180: one Cli layer for every present appearance flag (`--theme` // plus `--font-family`/`--font-size`/`--opacity`). Invalid appearance - // raws fail closed here with usage (exit 2 at the caller); the merge - // then only sees valid CLI input, so later merge errors stay - // file/profile-caused and keep their existing messages. + // raws (including an unknown `--theme`, CTX-1339) fail closed here with + // usage (exit 2 at the caller), so later merge errors are attributable + // to the merged layers rather than to CLI input. let cli = cli_overrides_from_args(args); if let Err(err) = cli.validate_appearance_overrides() { let flag = appearance_flag_for_field(err.field()); - return Err(format!("bitty: invalid {flag}: {err}\n{}", help_text())); + return Err(LoadFailure::fatal(format!( + "bitty: invalid {flag}: {err}\n{}", + help_text() + ))); } + // CTX-1339: a user file is only blameable for a merge failure when the + // merge succeeds without it. Re-merging without the user layer (profiles + // and CLI only) distinguishes a broken file from a profile/CLI error that + // merely surfaced while a valid file was present. The retry only runs on + // the failure path and only when a user file actually participated. + let retry_profiles = used_path.as_ref().map(|_| profile_layers.clone()); let merged = - bitty_config::file::resolve_effective_with_profiles(file_layer, profile_layers, &cli) - .map_err(|err| { - if let Some(p) = &used_path { - format!("bitty: invalid config '{}': {err}", p.display()) - } else if let Some(p) = &profile_path { - format!("bitty: invalid profile '{}': {err}", p.display()) - } else { - format!("bitty: invalid --theme: {err}") - } - })?; + match bitty_config::file::resolve_effective_with_profiles(file_layer, profile_layers, &cli) + { + Ok(merged) => merged, + Err(err) => { + let user_file_causal = retry_profiles.is_some_and(|profiles| { + bitty_config::file::resolve_effective_with_profiles(None, profiles, &cli) + .is_ok() + }); + return Err(classify_merge_failure( + &err, + used_path.as_deref(), + profile_path.as_deref(), + user_file_causal, + )); + } + }; Ok(LoadedConfig { merged, probed, @@ -296,7 +383,12 @@ pub(crate) fn load_merged_config(args: &Args) -> Result { /// impure (filesystem reads + env vars XDG/HOME/BITTY_*); total (all failures /// become `Err(String)`). pub(crate) fn load_app_config(args: &Args) -> Result { - let loaded = load_merged_config(args)?; + resolve_app_config(args).map_err(LoadFailure::into_message) +} + +/// Classified form of [`load_app_config`] for the startup fallback. +fn resolve_app_config(args: &Args) -> Result { + let loaded = resolve_merged_config(args)?; let merged = loaded.merged; let probed = loaded.probed; let profile_request = loaded.profile_name; @@ -369,6 +461,104 @@ pub(crate) fn load_app_config(args: &Args) -> Result { }) } +/// Result of the process-startup config resolution (CTX-1339, issue #2225). +pub(crate) struct StartupConfig { + /// The config startup continues with. + pub(crate) app: AppConfig, + /// User-facing diagnostics to print before startup proceeds; `None` when + /// the external configuration loaded normally. + pub(crate) diagnostics: Option, +} + +/// Whether this process has no last-good config plan (R-009 first start). +/// +/// The predicate is existing state, not a launch counter: the reload context +/// owns the last-good plan and is installed only after a successful startup +/// load (`config_reload::install`). At startup `current_effective()` is +/// `None`, so there is no plan to retain and the first-start fallback applies. +/// Once the context exists, every later failure keeps `engine.current()` +/// (`reload_into` reports `load-error` without replacing the plan), so the +/// fallback never runs there. +fn is_first_start() -> bool { + crate::config_reload::current_effective().is_none() +} + +/// Resolve the startup config, degrading to [`bitty_config::LayerKind::CoreDefaults`] +/// on first start. +/// +/// Pure over the classified load result and the predicate so both branches are +/// deterministically testable. `first_start` is [`is_first_start`] in +/// production; a later failure (an installed last-good plan) stays fail-closed +/// and returns the original message unchanged. +/// +/// Only a present-but-unusable user config file is recoverable. Explicitly +/// missing `--config`/`BITTY_CONFIG` paths, profile selection failures, CLI +/// usage errors, and an invalid built-in plan keep their fail-closed exit. +/// +/// The fallback is the unmodified built-in plan: no file, profile, CLI, or +/// safe-mode decoration participates, so startup never exits on unusable +/// external configuration and never claims values the user did not provide. +/// +/// # Errors +/// +/// Returns the original load message when no first-start fallback applies, or +/// a diagnostic if the built-in plan itself is invalid (defense in depth). +fn resolve_startup_config( + loaded: Result, + first_start: bool, +) -> Result { + match loaded { + Ok(app) => Ok(StartupConfig { + app, + diagnostics: None, + }), + Err(failure) if first_start && failure.fallback_allowed => Ok(StartupConfig { + app: builtin_app_config()?, + diagnostics: Some(format!( + "{}\nbitty: first start with unusable configuration; continuing with the built-in defaults (fix the configuration and restart; 'bitty config check' reports details)", + failure.message + )), + }), + Err(failure) => Err(failure.message), + } +} + +/// Loads the config for the process-startup path, applying the R-009 +/// first-start fallback. +/// +/// Startup-only: the reload resolver keeps calling [`load_app_config`], so a +/// later broken reload retains its last-good plan instead of replacing it with +/// built-ins. +/// +/// # Errors +/// +/// Same as [`resolve_startup_config`]. +pub(crate) fn load_startup_config(args: &Args) -> Result { + resolve_startup_config(resolve_app_config(args), is_first_start()) +} + +/// Builds the unmodified `CoreDefaults` plan used by the first-start fallback. +fn builtin_app_config() -> Result { + // An empty layer stack merges to built-in defaults with `CoreDefaults` + // attribution and runs the same validation as the normal merge. + let merged = bitty_config::merge_layers(Vec::new()) + .map_err(|err| format!("bitty: invalid built-in config: {err}"))?; + let effective = merged.effective; + let (theme, resolution) = bitty_config::theme::resolve_selection_with_status( + effective.appearance.theme.as_deref(), + startup_prefer_light(), + ); + Ok(AppConfig { + effective, + file_path: None, + profile_name: None, + profile_path: None, + theme, + resolution, + source: "default", + }) +} + /// Short usage for `bitty config` (stderr, fail-closed exit 2). pub(crate) fn config_usage() -> String { "usage: bitty config [--config PATH] [--profile NAME]\n\ @@ -1728,3 +1918,216 @@ fn animation_policy_from_effective( platform_reduced: false, } } + +#[cfg(test)] +mod startup_tests { + use super::*; + use bitty_config::EffectiveConfig; + + /// Builds the app bundle for a valid-config control without touching the + /// filesystem or process environment. + fn app_config(effective: EffectiveConfig) -> AppConfig { + let (theme, resolution) = bitty_config::theme::resolve_selection_with_status( + effective.appearance.theme.as_deref(), + false, + ); + AppConfig { + effective, + file_path: None, + profile_name: None, + profile_path: None, + theme, + resolution, + source: "default", + } + } + + /// Unique scratch directory for classifier tests (no sleeps). + fn scratch_dir(tag: &str) -> std::path::PathBuf { + let dir = std::env::temp_dir().join(format!( + "bitty-ctx1339-unit-{tag}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("clock after epoch") + .as_nanos() + )); + std::fs::create_dir_all(&dir).expect("scratch dir"); + dir + } + + #[test] + fn first_start_broken_config_uses_builtin_defaults_and_reports_diagnostics() { + let broken = String::from("bitty: invalid config file '/xdg/bitty/init.lua': syntax error"); + let startup = resolve_startup_config(Err(LoadFailure::broken(broken.clone())), true) + .expect("first start must proceed with the built-in config"); + + assert_eq!( + startup.app.effective, + EffectiveConfig::default(), + "the fallback must be the unmodified CoreDefaults plan" + ); + assert_eq!(startup.app.source, "default"); + assert!(startup.app.file_path.is_none()); + assert!(startup.app.profile_name.is_none()); + let diagnostics = startup + .diagnostics + .expect("fallback must report diagnostics"); + assert!(diagnostics.contains(&broken), "diagnostics: {diagnostics}"); + assert!( + diagnostics.contains("first start"), + "diagnostics: {diagnostics}" + ); + assert!( + diagnostics.contains("built-in defaults"), + "diagnostics: {diagnostics}" + ); + } + + #[test] + fn later_failure_with_a_last_good_plan_stays_fail_closed() { + let broken = String::from("bitty: invalid config file '/xdg/bitty/init.lua': syntax error"); + match resolve_startup_config(Err(LoadFailure::broken(broken.clone())), false) { + Ok(_) => panic!("an existing plan must be retained, never replaced by built-ins"), + Err(error) => assert_eq!(error, broken), + } + } + + #[test] + fn first_start_fatal_load_errors_stay_fail_closed() { + // Classification, not a fabricated LoadFailure: a CLI-layer merge + // error stays fatal even though first start has no last-good plan. + let err = bitty_config::ConfigError::validation("appearance.theme", "unknown theme 'x'"); + let failure = classify_merge_failure(&err, None, None, false); + let message = failure.message.clone(); + match resolve_startup_config(Err(failure), true) { + Ok(_) => panic!("a CLI-layer failure must never fall back to built-ins"), + Err(error) => assert_eq!(error, message), + } + } + + #[test] + fn merge_failure_attribution_never_blames_a_valid_user_file() { + let err = bitty_config::ConfigError::validation("appearance.theme", "unknown theme 'x'"); + let file = std::path::Path::new("/xdg/bitty/init.lua"); + let profile = std::path::Path::new("/xdg/bitty/profiles/bad.lua"); + + // CLI error while a valid file participates: file is not causal. + let cli = classify_merge_failure(&err, Some(file), None, false); + assert!(!cli.fallback_allowed, "{}", cli.message); + assert!( + cli.message.starts_with("bitty: invalid --theme:"), + "{}", + cli.message + ); + + // Profile error while a valid file participates: attributed to the + // profile, never to the file. + let profile_failure = classify_merge_failure(&err, Some(file), Some(profile), false); + assert!( + !profile_failure.fallback_allowed, + "{}", + profile_failure.message + ); + assert!( + profile_failure.message.contains("invalid profile"), + "{}", + profile_failure.message + ); + assert!( + profile_failure.message.contains("profiles/bad.lua"), + "{}", + profile_failure.message + ); + + // A re-merge without the user layer succeeded: the file is causal. + let broken = classify_merge_failure(&err, Some(file), None, true); + assert!(broken.fallback_allowed, "{}", broken.message); + assert!(broken.message.contains("init.lua"), "{}", broken.message); + } + + #[test] + fn classifier_marks_a_present_broken_file_recoverable() { + let dir = scratch_dir("broken-file"); + let path = dir.join("init.lua"); + std::fs::write(&path, "return { font = }\n").expect("write broken init.lua"); + let mut args = Args::new(); + args.config_path = Some(path.to_string_lossy().into_owned()); + + let failure = resolve_app_config(&args) + .err() + .expect("a present-but-broken file must fail to load"); + assert!(failure.fallback_allowed, "{}", failure.message); + assert!( + failure.message.contains("invalid config file"), + "{}", + failure.message + ); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn classifier_keeps_a_missing_explicit_path_fatal() { + let dir = scratch_dir("missing-path"); + let mut args = Args::new(); + args.config_path = Some(dir.join("missing.lua").to_string_lossy().into_owned()); + + let failure = resolve_app_config(&args) + .err() + .expect("an explicitly missing path must fail"); + assert!(!failure.fallback_allowed, "{}", failure.message); + assert!(failure.message.contains("not found"), "{}", failure.message); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn classifier_keeps_invalid_cli_theme_fatal_with_a_valid_file() { + let dir = scratch_dir("bad-theme"); + let path = dir.join("init.lua"); + std::fs::write(&path, "return { appearance = { theme = \"dracula\" } }\n") + .expect("write valid init.lua"); + let mut args = Args::new(); + args.config_path = Some(path.to_string_lossy().into_owned()); + args.theme = Some(String::from("not-a-theme")); + + let failure = resolve_app_config(&args) + .err() + .expect("an unknown --theme must fail closed"); + assert!(!failure.fallback_allowed, "{}", failure.message); + assert!( + failure.message.contains("invalid --theme"), + "{}", + failure.message + ); + assert!( + failure.message.contains("appearance.theme"), + "{}", + failure.message + ); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn valid_config_is_returned_unchanged() { + let mut effective = EffectiveConfig::default(); + effective.font.size = 15.0; + let startup = resolve_startup_config(Ok(app_config(effective.clone())), true) + .expect("valid config must load"); + assert_eq!(startup.app.effective, effective); + assert!(startup.diagnostics.is_none()); + } + + #[test] + fn first_start_predicate_tracks_the_installed_last_good_plan() { + crate::config_reload::clear(); + assert!(is_first_start(), "startup has no plan to retain"); + + let plan = EffectiveConfig::default(); + crate::config_reload::install_with(plan, Box::new(|| Err(String::from("unused"))), None); + assert!( + !is_first_start(), + "an installed last-good plan means later failures retain it" + ); + crate::config_reload::clear(); + } +} diff --git a/crates/bitty-terminal/src/config_reload.rs b/crates/bitty-terminal/src/config_reload.rs index 29a16b611..bdf24ec60 100644 --- a/crates/bitty-terminal/src/config_reload.rs +++ b/crates/bitty-terminal/src/config_reload.rs @@ -1537,6 +1537,34 @@ mod tests { clear(); } + #[test] + fn broken_reload_retains_the_last_good_plan() { + // CTX-1339 (#2225): startup fell back to built-ins; a later broken + // reload must keep that last-good plan instead of replacing it. + let mut baseline = bitty_config::EffectiveConfig::default(); + baseline.font.size = 13.5; + clear(); + install_with( + baseline.clone(), + Box::new(|| { + Err(String::from( + "bitty: invalid config file '/xdg/bitty/init.lua': syntax error", + )) + }), + None, + ); + let mut runtime = bitty_runtime::Runtime::with_defaults().expect("runtime"); + let info = reload_requested(&mut runtime).expect("context installed"); + assert_eq!(info.kind, "load-error"); + assert!(!info.applied); + assert_eq!( + current_effective(), + Some(baseline), + "a broken reload must retain the last good plan" + ); + clear(); + } + #[test] fn classification_marks_font_size_live() { // Guard the assumption the engine relies on: font.size is a live field diff --git a/crates/bitty-terminal/src/main.rs b/crates/bitty-terminal/src/main.rs index 7115528df..6e4879602 100644 --- a/crates/bitty-terminal/src/main.rs +++ b/crates/bitty-terminal/src/main.rs @@ -265,7 +265,7 @@ use cli::{ help_valid_commands, parse_args, }; use config_cli::{ - config_usage, load_app_config, run_config_subcommand, runtime_config_from_effective_for, + config_usage, load_startup_config, run_config_subcommand, runtime_config_from_effective_for, startup_prefer_light, }; use init::run_init_subcommand; @@ -812,15 +812,23 @@ fn main() { std::process::exit(x::run_cli(&args)); } - // User config first (fail-closed): invalid files exit non-zero with a - // clear stderr message; missing default-path files yield defaults. - let app_config = match load_app_config(&args) { - Ok(cfg) => cfg, + // User config first. CTX-1339 (#2225, R-009): first start with unusable + // configuration proceeds on the unmodified built-in `CoreDefaults` plan + // and reports diagnostics; there is no last-good plan to retain yet. Once + // `config_reload::install` seeds a plan, later failures keep it and never + // reach this startup fallback. A non-first-start error stays fail-closed. + // Missing default-path files still yield defaults. + let startup = match load_startup_config(&args) { + Ok(startup) => startup, Err(msg) => { eprintln!("{msg}"); std::process::exit(2); } }; + if let Some(diagnostics) = &startup.diagnostics { + eprintln!("{diagnostics}"); + } + let app_config = startup.app; logging::info(|| { format!( "bitty: theme '{}' (resolution={:?} source={})", diff --git a/crates/bitty-terminal/src/tests.rs b/crates/bitty-terminal/src/tests.rs index cbb3bb928..54d740b80 100644 --- a/crates/bitty-terminal/src/tests.rs +++ b/crates/bitty-terminal/src/tests.rs @@ -2919,6 +2919,10 @@ fn cli_overrides_from_args_trims_and_passes_raws() { assert!(cli.is_empty()); assert!(cli.validate_appearance_overrides().is_ok()); // Flag naming for the fail-closed message. + assert_eq!( + appearance_flag_for_field(Some("appearance.theme")), + "--theme" + ); assert_eq!(appearance_flag_for_field(Some("font.size")), "--font-size"); assert_eq!( appearance_flag_for_field(Some("window.opacity")), diff --git a/crates/bitty-terminal/tests/cli_config_fallback.rs b/crates/bitty-terminal/tests/cli_config_fallback.rs new file mode 100644 index 000000000..bb5a5cff3 --- /dev/null +++ b/crates/bitty-terminal/tests/cli_config_fallback.rs @@ -0,0 +1,271 @@ +//! Binary-level first-start config fallback proof (CTX-1339, issue #2225). +//! +//! R-009 contract: on first start with missing or broken configuration Bitty +//! proceeds on the minimal built-in (`CoreDefaults`) configuration and reports +//! diagnostics; later failures retain the last good plan. These tests drive +//! the built `bitty` binary with an isolated `XDG_CONFIG_HOME` and the +//! deterministic `--headless` path (no display, instance, or plugin VM). + +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; + +/// Binary under test (fails to compile if the `[[bin]]` rename regresses). +const BITTY_BIN: &str = env!("CARGO_BIN_EXE_bitty"); + +/// Fresh isolated scratch root (config + HOME + state + data). +fn scratch_root(tag: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "bitty-ctx1339-{tag}-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("clock after epoch") + .as_nanos() + )); + std::fs::create_dir_all(dir.join("bitty").join("profiles")).expect("config dir"); + dir +} + +/// Writes `$XDG_CONFIG_HOME/bitty/init.lua` with the given body. +fn write_init(root: &Path, body: &str) { + std::fs::write(root.join("bitty").join("init.lua"), body).expect("write init.lua"); +} + +/// Writes `$XDG_CONFIG_HOME/bitty/profiles/.lua` with the given body. +fn write_profile(root: &Path, name: &str, body: &str) { + std::fs::write( + root.join("bitty") + .join("profiles") + .join(format!("{name}.lua")), + body, + ) + .expect("write profile"); +} + +/// Runs `bitty ` under the isolated root. +fn run_bitty(root: &Path, args: &[&str]) -> Output { + run_bitty_with_env(root, args, &[]) +} + +/// Runs `bitty ` under the isolated root with extra environment values. +fn run_bitty_with_env(root: &Path, args: &[&str], envs: &[(&str, &str)]) -> Output { + let mut command = Command::new(BITTY_BIN); + command + .args(args) + .env("XDG_CONFIG_HOME", root) + .env("XDG_STATE_HOME", root.join("state")) + .env("XDG_DATA_HOME", root.join("data")) + .env("HOME", root) + .env("NO_COLOR", "1") + .env_remove("BITTY_CONFIG") + .env_remove("BITTY_PROFILE"); + for (key, value) in envs { + command.env(key, value); + } + command + .output() + .unwrap_or_else(|err| panic!("spawn {BITTY_BIN:?} {args:?}: {err}")) +} + +fn stdout(output: &Output) -> String { + String::from_utf8_lossy(&output.stdout).into_owned() +} + +fn stderr(output: &Output) -> String { + String::from_utf8_lossy(&output.stderr).into_owned() +} + +#[test] +fn first_start_with_broken_config_proceeds_with_builtin_defaults() { + let root = scratch_root("broken"); + write_init(&root, "return { font = }\n"); + + let output = run_bitty(&root, &["--headless"]); + assert_eq!( + output.status.code(), + Some(0), + "first start must proceed on built-ins, stderr={:?}", + stderr(&output) + ); + let err = stderr(&output); + assert!( + err.contains("invalid config file"), + "the load diagnostic must be reported, stderr={err:?}" + ); + assert!( + err.contains("first start") && err.contains("built-in defaults"), + "the fallback must be reported, stderr={err:?}" + ); + let text = stdout(&output); + assert!( + text.contains("source=default") && text.contains("file=(none)"), + "built-in defaults must be in effect, stdout={text:?}" + ); + + let _ = std::fs::remove_dir_all(&root); +} + +#[test] +fn valid_config_startup_is_unchanged() { + let root = scratch_root("valid"); + write_init(&root, "return { appearance = { theme = \"dracula\" } }\n"); + + let output = run_bitty(&root, &["--headless"]); + assert_eq!( + output.status.code(), + Some(0), + "valid config must start, stderr={:?}", + stderr(&output) + ); + let text = stdout(&output); + assert!( + text.contains("theme 'dracula'") && text.contains("source=file"), + "the file layer must still win, stdout={text:?}" + ); + assert!( + !stderr(&output).contains("built-in defaults"), + "a valid config must not report the fallback" + ); + + let _ = std::fs::remove_dir_all(&root); +} + +#[test] +fn missing_explicit_config_still_fails_closed() { + let root = scratch_root("explicit-missing"); + let missing = root.join("missing.lua"); + let missing = missing.to_str().expect("utf-8 scratch path"); + + let output = run_bitty(&root, &["--config", missing, "--headless"]); + assert_eq!( + output.status.code(), + Some(2), + "an explicitly missing --config must stay fail-closed, stderr={:?}", + stderr(&output) + ); + let err = stderr(&output); + assert!(err.contains("not found"), "stderr={err:?}"); + assert!( + !err.contains("built-in defaults"), + "an explicit missing path is not a first-start fallback, stderr={err:?}" + ); + + let _ = std::fs::remove_dir_all(&root); +} + +#[test] +fn missing_bitty_config_env_still_fails_closed() { + let root = scratch_root("env-missing"); + let missing = root.join("missing.lua"); + let missing = missing.to_str().expect("utf-8 scratch path"); + + let output = run_bitty_with_env(&root, &["--headless"], &[("BITTY_CONFIG", missing)]); + assert_eq!( + output.status.code(), + Some(2), + "an explicitly missing BITTY_CONFIG must stay fail-closed, stderr={:?}", + stderr(&output) + ); + let err = stderr(&output); + assert!(err.contains("BITTY_CONFIG"), "stderr={err:?}"); + assert!(err.contains("not found"), "stderr={err:?}"); + assert!( + !err.contains("built-in defaults"), + "an explicit missing path is not a first-start fallback, stderr={err:?}" + ); + assert!( + !stdout(&output).contains("bitty headless:"), + "startup must not proceed with built-ins" + ); + + let _ = std::fs::remove_dir_all(&root); +} + +#[test] +fn invalid_theme_with_valid_file_stays_fail_closed() { + let root = scratch_root("invalid-theme"); + write_init(&root, "return { appearance = { theme = \"dracula\" } }\n"); + + let output = run_bitty(&root, &["--theme", "not-a-theme", "--headless"]); + assert_eq!( + output.status.code(), + Some(2), + "an unknown --theme is a CLI usage error, stderr={:?}", + stderr(&output) + ); + let err = stderr(&output); + assert!(err.contains("invalid --theme"), "stderr={err:?}"); + assert!(err.contains("appearance.theme"), "stderr={err:?}"); + assert!( + !err.contains("built-in defaults"), + "a valid user file must not absorb a CLI error, stderr={err:?}" + ); + assert!( + !stdout(&output).contains("bitty headless:"), + "startup must not proceed with built-ins" + ); + + let _ = std::fs::remove_dir_all(&root); +} + +#[test] +fn invalid_profile_with_valid_file_stays_fail_closed() { + let root = scratch_root("invalid-profile"); + write_init(&root, "return { appearance = { theme = \"dracula\" } }\n"); + // Passes profile-layer parsing/validation but fails the merged outline + // contrast contract (AC-1) on the default dark background. + write_profile( + &root, + "bad", + "return { decoration = { border_color_focused = \"#000000\" } }\n", + ); + + let output = run_bitty(&root, &["--profile", "bad", "--headless"]); + assert_eq!( + output.status.code(), + Some(2), + "an invalid profile must stay fail-closed, stderr={:?}", + stderr(&output) + ); + let err = stderr(&output); + assert!(err.contains("invalid profile"), "stderr={err:?}"); + assert!(err.contains("bad.lua"), "stderr={err:?}"); + assert!( + !err.contains("init.lua"), + "the valid user file must not be blamed, stderr={err:?}" + ); + assert!( + !err.contains("built-in defaults"), + "a valid user file must not absorb a profile error, stderr={err:?}" + ); + assert!( + !stdout(&output).contains("bitty headless:"), + "startup must not proceed with built-ins" + ); + + let _ = std::fs::remove_dir_all(&root); +} + +#[test] +fn missing_config_startup_is_unchanged() { + let root = scratch_root("missing"); + + let output = run_bitty(&root, &["--headless"]); + assert_eq!( + output.status.code(), + Some(0), + "a missing default-path config already yields defaults, stderr={:?}", + stderr(&output) + ); + let text = stdout(&output); + assert!( + text.contains("source=default") && text.contains("file=(none)"), + "missing config must keep the default path, stdout={text:?}" + ); + assert!( + !stderr(&output).contains("built-in defaults"), + "a missing default config is not a fallback" + ); + + let _ = std::fs::remove_dir_all(&root); +}