From f5c97832eab1987c7ad1613bcbca22e76e8a8182 Mon Sep 17 00:00:00 2001 From: Paul O'Fallon Date: Wed, 22 Jul 2026 22:49:55 +0000 Subject: [PATCH] feat(cli): migrate --log-format/--force-tty-if-json to clap-native env= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Category B of #180 (part 2). Move the last two flag-backed env vars onto clap's `env=` and, in doing so, fix a latent flag-vs-env bug in `up`. - `--log-format` gains `env = "DEACON_LOG_FORMAT"`; `is_json_log_format()` drops its manual env fallback and just reads the clap-resolved field. - `--force-tty-if-json` gains `env = "DEACON_FORCE_TTY_IF_JSON"`; `resolve_force_pty` drops its manual env branch (clap boolish parse owns it). - Latent bug fix: `up`'s lifecycle and compose PTY gating re-read `DEACON_LOG_FORMAT` from the environment directly, so `--log-format json` passed as a *flag* (no env var) never triggered json-mode there. Thread the clap-resolved value through a new `UpArgs.json_log_format` field instead; the deep sites now read `args.json_log_format`. Removed the now-dead `ENV_LOG_FORMAT` const (`ENV_FORCE_TTY_IF_JSON` stays — it is injected into the container env, which is out of #180 scope). - `LogFormat` gains `PartialEq`/`Eq`; temp-env parse tests (env / flag-beats-env / falsey) added; three existing env-dependent tests now pin the env explicitly so they stay deterministic under a shared-process runner too. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_019vhr7Tcf8ybvSZSE1owqBT --- crates/deacon/src/cli.rs | 117 ++++++++++++++++----- crates/deacon/src/commands/up/args.rs | 7 ++ crates/deacon/src/commands/up/compose.rs | 9 +- crates/deacon/src/commands/up/lifecycle.rs | 49 +++------ crates/deacon/src/commands/up/mod.rs | 3 - 5 files changed, 117 insertions(+), 68 deletions(-) diff --git a/crates/deacon/src/cli.rs b/crates/deacon/src/cli.rs index a5dac943..dbf3d732 100644 --- a/crates/deacon/src/cli.rs +++ b/crates/deacon/src/cli.rs @@ -120,7 +120,7 @@ pub enum OutputFormat { } /// Log format options -#[derive(Debug, Clone, ValueEnum)] +#[derive(Debug, Clone, ValueEnum, PartialEq, Eq)] pub enum LogFormat { /// Human-readable text format Text, @@ -879,7 +879,7 @@ pub enum ConfigCommands { )] pub struct Cli { /// Log format (text or json, defaults to text, can be set via DEACON_LOG_FORMAT env var) - #[arg(long, global = true, value_enum)] + #[arg(long, global = true, value_enum, env = "DEACON_LOG_FORMAT")] pub log_format: Option, /// Log level (baseline verbosity). Shifted by repeated `-v`/`-q`. @@ -1025,7 +1025,7 @@ pub struct Cli { /// /// When disabled (default), lifecycle commands run without PTY allocation. This is suitable /// for non-interactive scripts and automated environments. - #[arg(long, global = true)] + #[arg(long, global = true, env = "DEACON_FORCE_TTY_IF_JSON")] pub force_tty_if_json: bool, /// Default user env probe mode (none|loginInteractiveShell|interactiveShell|loginShell) @@ -1142,16 +1142,12 @@ impl Cli { /// assert!(ctx.workspace_folder.is_none()); /// ``` /// Returns true when the effective log format is JSON. - /// `--log-format json` wins; if unset, `DEACON_LOG_FORMAT=json` counts too - /// (matches the fallback in deacon_core::logging::init). + /// + /// `self.log_format` already reflects flag-over-env-over-unset: clap's `env=` + /// on `--log-format` resolves `DEACON_LOG_FORMAT` into the field, so no manual + /// env read is needed here. pub fn is_json_log_format(&self) -> bool { - match self.log_format { - Some(LogFormat::Json) => true, - Some(LogFormat::Text) => false, - None => std::env::var("DEACON_LOG_FORMAT") - .map(|v| v == "json") - .unwrap_or(false), - } + matches!(self.log_format, Some(LogFormat::Json)) } #[allow(dead_code)] // Reserved for future command implementations; see runtime_utils @@ -1423,6 +1419,7 @@ impl Cli { // JSON log format auto-forces PTY allocation so lifecycle exec output // stays usable; the explicit flag remains as a manual override. force_tty_if_json: self.force_tty_if_json || json_format, + json_log_format: json_format, trust_workspace: self.trust_workspace, trust_workspace_persist: self.trust_workspace_persist, host_ca_activation, @@ -2018,33 +2015,99 @@ mod tests { /// BEAD-11-T03: --log-format json must auto-force PTY allocation so the /// downstream ExecArgs.force_tty_if_json is true even without the explicit flag. + /// + /// Both fields now read the environment via clap `env=` (#180), so the env is + /// pinned explicitly to keep the assertion deterministic under any test runner. #[test] fn test_json_log_format_implies_force_tty() { - let cli = Cli::parse_from(["deacon", "--log-format", "json"]); - assert!(cli.is_json_log_format()); - assert!(!cli.force_tty_if_json); // user didn't pass the explicit flag - // The dispatch site ORs these two together; the test of that wiring - // is here at the source of truth (cli.is_json_log_format()) since - // dispatch is async and harder to unit-test in isolation. - let effective_force_tty = cli.force_tty_if_json || cli.is_json_log_format(); - assert!(effective_force_tty); + temp_env::with_vars( + [ + ("DEACON_LOG_FORMAT", None::<&str>), + ("DEACON_FORCE_TTY_IF_JSON", None::<&str>), + ], + || { + let cli = Cli::parse_from(["deacon", "--log-format", "json"]); + assert!(cli.is_json_log_format()); + assert!(!cli.force_tty_if_json); // user didn't pass the explicit flag + // The dispatch site ORs these two together; the test of that wiring + // is here at the source of truth (cli.is_json_log_format()) since + // dispatch is async and harder to unit-test in isolation. + let effective_force_tty = cli.force_tty_if_json || cli.is_json_log_format(); + assert!(effective_force_tty); + }, + ); } /// Inverse: explicit --log-format text leaves the auto-derive off. #[test] fn test_text_log_format_does_not_imply_force_tty() { - let cli = Cli::parse_from(["deacon", "--log-format", "text"]); - assert!(!cli.is_json_log_format()); - let effective_force_tty = cli.force_tty_if_json || cli.is_json_log_format(); - assert!(!effective_force_tty); + temp_env::with_vars( + [ + ("DEACON_LOG_FORMAT", None::<&str>), + ("DEACON_FORCE_TTY_IF_JSON", None::<&str>), + ], + || { + let cli = Cli::parse_from(["deacon", "--log-format", "text"]); + assert!(!cli.is_json_log_format()); + let effective_force_tty = cli.force_tty_if_json || cli.is_json_log_format(); + assert!(!effective_force_tty); + }, + ); } /// Explicit --force-tty-if-json still works without --log-format json. #[test] fn test_explicit_force_tty_without_json_log_format() { - let cli = Cli::parse_from(["deacon", "--force-tty-if-json"]); - assert!(cli.force_tty_if_json); - assert!(!cli.is_json_log_format()); + temp_env::with_var_unset("DEACON_LOG_FORMAT", || { + let cli = Cli::parse_from(["deacon", "--force-tty-if-json"]); + assert!(cli.force_tty_if_json); + assert!(!cli.is_json_log_format()); + }); + } + + // --- #180 Category B (part 2): clap-native env= for --log-format / --force-tty-if-json --- + + #[test] + fn test_log_format_defaults_none() { + temp_env::with_var_unset("DEACON_LOG_FORMAT", || { + let cli = Cli::parse_from(["deacon"]); + assert_eq!(cli.log_format, None); + assert!(!cli.is_json_log_format()); + }); + } + + #[test] + fn test_log_format_reads_env_var() { + temp_env::with_var("DEACON_LOG_FORMAT", Some("json"), || { + let cli = Cli::parse_from(["deacon"]); + assert_eq!(cli.log_format, Some(LogFormat::Json)); + assert!(cli.is_json_log_format()); + }); + } + + #[test] + fn test_log_format_flag_beats_env_var() { + temp_env::with_var("DEACON_LOG_FORMAT", Some("json"), || { + let cli = Cli::parse_from(["deacon", "--log-format", "text"]); + assert_eq!(cli.log_format, Some(LogFormat::Text)); + assert!(!cli.is_json_log_format()); + }); + } + + #[test] + fn test_force_tty_reads_env_var() { + temp_env::with_var("DEACON_FORCE_TTY_IF_JSON", Some("true"), || { + let cli = Cli::parse_from(["deacon"]); + assert!(cli.force_tty_if_json); + }); + } + + #[test] + fn test_force_tty_env_falsey_stays_off() { + temp_env::with_var("DEACON_FORCE_TTY_IF_JSON", Some("false"), || { + let cli = Cli::parse_from(["deacon"]); + assert!(!cli.force_tty_if_json); + }); } /// Helper for the BEAD-07 tests: parse an exec invocation and pull out the diff --git a/crates/deacon/src/commands/up/args.rs b/crates/deacon/src/commands/up/args.rs index 23ac8cd8..7af75c78 100644 --- a/crates/deacon/src/commands/up/args.rs +++ b/crates/deacon/src/commands/up/args.rs @@ -484,6 +484,12 @@ pub struct UpArgs { pub secret_registry: deacon_core::redaction::SecretRegistry, pub force_tty_if_json: bool, + /// Whether the effective log format is JSON (resolved by clap from + /// `--log-format`/`DEACON_LOG_FORMAT` at the CLI tier). Threaded here so the + /// lifecycle/compose PTY gating no longer re-reads the env var itself, which + /// missed `--log-format json` passed as a flag (#180). + pub json_log_format: bool, + /// `--trust-workspace` flag (one-shot trust, does not persist). pub trust_workspace: bool, /// `--trust-workspace-persist` flag (one-shot + writes to the trust store). @@ -555,6 +561,7 @@ impl Default for UpArgs { redaction_config: deacon_core::redaction::RedactionConfig::default(), secret_registry: deacon_core::redaction::global_registry().clone(), force_tty_if_json: false, + json_log_format: false, trust_workspace: false, trust_workspace_persist: false, host_ca_activation: deacon_core::host_ca::HostCaActivation::Off, diff --git a/crates/deacon/src/commands/up/compose.rs b/crates/deacon/src/commands/up/compose.rs index 1b35c47a..2f0d6ef7 100644 --- a/crates/deacon/src/commands/up/compose.rs +++ b/crates/deacon/src/commands/up/compose.rs @@ -5,7 +5,6 @@ //! - `execute_compose_post_create` - Post-create lifecycle for compose //! - `handle_compose_shutdown` - Shutdown handling for compose -use super::ENV_LOG_FORMAT; use super::args::{MountType, NormalizedMount, UpArgs}; use super::features_build::{ FeatureBuildOutput, build_image_with_features, build_image_with_features_from_dockerfile, @@ -537,11 +536,9 @@ pub(crate) async fn execute_compose_up( // Execute post-create lifecycle if not skipped if !args.skip_post_create { - // Resolve PTY preference for compose post-create (same logic as lifecycle commands) - let json_mode = std::env::var(ENV_LOG_FORMAT) - .map(|v| v == "json") - .unwrap_or(false); - let force_pty = resolve_force_pty(args.force_tty_if_json, json_mode); + // Resolve PTY preference for compose post-create (same logic as lifecycle commands). + // json_mode is threaded from the clap-resolved --log-format/DEACON_LOG_FORMAT (#180). + let force_pty = resolve_force_pty(args.force_tty_if_json, args.json_log_format); execute_compose_post_create( &project, config, diff --git a/crates/deacon/src/commands/up/lifecycle.rs b/crates/deacon/src/commands/up/lifecycle.rs index b61bf752..a58645ff 100644 --- a/crates/deacon/src/commands/up/lifecycle.rs +++ b/crates/deacon/src/commands/up/lifecycle.rs @@ -1,14 +1,13 @@ //! Lifecycle command execution for the up command. //! //! This module contains: -//! - `resolve_force_pty` - Resolve PTY preference based on flags and environment +//! - `resolve_force_pty` - Resolve PTY preference based on the resolved flag and JSON mode //! - `build_invocation_context` - Build InvocationContext from CLI args and prior state //! - `execute_lifecycle_commands` - Execute lifecycle phases in container //! - `execute_initialize_command` - Execute initializeCommand on host (with workspace-trust gate) //! - `HostTrustArgs` / `enforce_host_trust` - Workspace-trust gate primitives use super::args::UpArgs; -use super::{ENV_FORCE_TTY_IF_JSON, ENV_LOG_FORMAT}; use anyhow::{Context, Result}; use deacon_core::config::DevContainerConfig; use deacon_core::container_lifecycle::{ @@ -25,27 +24,15 @@ use std::path::{Path, PathBuf}; use std::time::Duration; use tracing::{Level, debug, info, instrument, span, warn}; -/// Resolve PTY preference for lifecycle commands based on flag, environment, and JSON mode +/// Resolve PTY preference for lifecycle commands based on the resolved flag and JSON mode. /// -/// Per FR-002, FR-005: PTY toggle only applies in JSON log mode. -/// Precedence: CLI flag > env var `DEACON_FORCE_TTY_IF_JSON` > default (false) +/// Per FR-002, FR-005: PTY toggle only applies in JSON log mode. `flag` already +/// reflects `--force-tty-if-json`/`DEACON_FORCE_TTY_IF_JSON` — clap's `env=` +/// resolves the env var into the field at the CLI tier (#180), so no manual env +/// read is needed here. pub(crate) fn resolve_force_pty(flag: bool, json_mode: bool) -> bool { - // PTY toggle only applies when in JSON log mode - if !json_mode { - return false; - } - - // CLI flag takes precedence - if flag { - return true; - } - - // Check environment variable (truthy: true/1/yes; falsey: false/0/no or unset) - if let Ok(val) = std::env::var(ENV_FORCE_TTY_IF_JSON) { - matches!(val.to_lowercase().as_str(), "true" | "1" | "yes") - } else { - false // default: no PTY - } + // PTY toggle only applies when in JSON log mode. + json_mode && flag } /// Build an `InvocationContext` from CLI arguments and prior state markers. @@ -207,21 +194,19 @@ pub(crate) async fn execute_lifecycle_commands( args.mount_workspace_git_root, ); - // Determine if JSON log mode is active by checking DEACON_LOG_FORMAT env var - // Per FR-001, FR-002: PTY toggle only applies in JSON log mode - let json_mode = std::env::var(ENV_LOG_FORMAT) - .map(|v| v == "json") - .unwrap_or(false); + // Whether JSON log mode is active — resolved by clap from + // `--log-format`/`DEACON_LOG_FORMAT` at the CLI tier and threaded via + // UpArgs. Per FR-001, FR-002: PTY toggle only applies in JSON log mode. + // (Previously re-read the env var here, which missed `--log-format json` + // passed as a flag — #180.) + let json_mode = args.json_log_format; - // Resolve PTY preference based on flag, env, and JSON mode + // Resolve PTY preference based on flag and JSON mode. let force_pty = resolve_force_pty(args.force_tty_if_json, json_mode); debug!( - "PTY preference resolved: force_pty={}, json_mode={}, flag={}, env={}", - force_pty, - json_mode, - args.force_tty_if_json, - std::env::var(ENV_FORCE_TTY_IF_JSON).unwrap_or_else(|_| "unset".to_string()) + "PTY preference resolved: force_pty={}, json_mode={}, flag={}", + force_pty, json_mode, args.force_tty_if_json, ); let wait_for = wait_for_phase(config.wait_for.as_deref())?; diff --git a/crates/deacon/src/commands/up/mod.rs b/crates/deacon/src/commands/up/mod.rs index 231fefb1..281572bb 100644 --- a/crates/deacon/src/commands/up/mod.rs +++ b/crates/deacon/src/commands/up/mod.rs @@ -74,9 +74,6 @@ use merged_config::merge_image_metadata_into_config; /// for lifecycle exec commands during `deacon up` when JSON logging is active. pub(crate) const ENV_FORCE_TTY_IF_JSON: &str = "DEACON_FORCE_TTY_IF_JSON"; -/// Environment variable name for log format detection. -pub(crate) const ENV_LOG_FORMAT: &str = "DEACON_LOG_FORMAT"; - /// Starts development containers for the current workspace according to the resolved devcontainer configuration. /// /// This is the top-level entry point for the `up` command. It: