From aa0d7600392841117354c77d161eb49bb1b98ef7 Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Tue, 23 Jun 2026 06:25:19 +0000 Subject: [PATCH 1/2] refactor(cli): extract doctor diagnostics boundary --- crates/git-smee-cli/src/doctor.rs | 255 ++++++++++++++++++++++++++++++ crates/git-smee-cli/src/main.rs | 221 +------------------------- 2 files changed, 258 insertions(+), 218 deletions(-) create mode 100644 crates/git-smee-cli/src/doctor.rs diff --git a/crates/git-smee-cli/src/doctor.rs b/crates/git-smee-cli/src/doctor.rs new file mode 100644 index 0000000..273e00f --- /dev/null +++ b/crates/git-smee-cli/src/doctor.rs @@ -0,0 +1,255 @@ +use std::{env, fs, path::Path}; + +use git_smee_core::{installer, installer::MANAGED_FILE_MARKER, repository}; +use serde::Serialize; + +use crate::{normalize_config_path_for_hook_script, read_config_file}; + +#[derive(Debug, Serialize)] +struct DoctorReport { + status: DoctorStatus, + repository_root: Option, + hooks_dir: Option, + config_path: String, + ok: Vec, + warnings: Vec, + errors: Vec, +} + +#[derive(Debug, Serialize)] +#[serde(rename_all = "lowercase")] +enum DoctorStatus { + Ok, + Warning, + Error, +} + +pub(crate) fn run_doctor(config_path: &Path, json: bool) -> Result<(), Box> { + let report = build_doctor_report(config_path); + if json { + println!("{}", serde_json::to_string_pretty(&report)?); + } else { + print_doctor_report(&report); + } + if report.errors.is_empty() { + Ok(()) + } else { + Err("doctor found repository setup errors".into()) + } +} + +fn build_doctor_report(config_path: &Path) -> DoctorReport { + let mut report = DoctorReport { + status: DoctorStatus::Ok, + repository_root: None, + hooks_dir: None, + config_path: config_path.display().to_string(), + ok: Vec::new(), + warnings: Vec::new(), + errors: Vec::new(), + }; + + let repository_root = match repository::find_git_root() { + Ok(root) => { + report.ok.push("inside a Git repository".to_string()); + report.repository_root = Some(root.display().to_string()); + root + } + Err(error) => { + report.errors.push(format!( + "not inside a Git repository; run git smee doctor from a repository ({error})" + )); + return finish_doctor_report(report); + } + }; + + let hooks_dir = match repository::resolve_git_path( + &repository_root, + installer::FileSystemHookInstaller::HOOKS_GIT_PATH_KEY, + ) { + Ok(path) => { + report.hooks_dir = Some(path.display().to_string()); + if path.exists() && path.is_dir() { + report + .ok + .push(format!("hooks directory exists at {}", path.display())); + } else if path.exists() { + report.errors.push(format!( + "effective hooks path is not a directory: {}; fix core.hooksPath or remove the file", + path.display() + )); + } else { + report.warnings.push(format!( + "hooks directory does not exist yet at {}; run git smee install to create it", + path.display() + )); + } + path + } + Err(error) => { + report.errors.push(format!( + "could not resolve effective hooks directory; check git core.hooksPath ({error})" + )); + return finish_doctor_report(report); + } + }; + + let config = match read_config_file(config_path) { + Ok(config) => { + report + .ok + .push(format!("config parses from {}", config_path.display())); + if config.hooks.is_empty() { + report.errors.push( + "configuration contains no hooks; add at least one [[hook-name]] entry" + .to_string(), + ); + } else { + report.ok.push(format!( + "{} configured hook phase(s) are valid", + config.hooks.len() + )); + } + config + } + Err(error) => { + report.errors.push(format!( + "config problem at {}: {error}; run git smee init or fix the TOML file", + config_path.display() + )); + return finish_doctor_report(report); + } + }; + + let expected_config_path = normalize_config_path_for_hook_script(config_path, &repository_root) + .unwrap_or_else(|_| config_path.to_path_buf()); + let expected_exe = env::current_exe().ok(); + let expected_config = expected_config_path.to_string_lossy().to_string(); + + let mut phases: Vec<_> = config.hooks.keys().copied().collect(); + phases.sort_by_key(|phase| phase.as_str()); + for phase in phases { + let hook_path = hooks_dir.join(phase.as_str()); + if !hook_path.exists() { + report.errors.push(format!( + "missing managed wrapper for {phase} at {}; run git smee install", + hook_path.display() + )); + continue; + } + if !hook_path.is_file() { + report.errors.push(format!( + "hook path for {phase} is not a regular file: {}; remove it or fix core.hooksPath", + hook_path.display() + )); + continue; + } + let content = match fs::read_to_string(&hook_path) { + Ok(content) => content, + Err(error) => { + report.errors.push(format!( + "cannot read hook wrapper for {phase} at {}: {error}", + hook_path.display() + )); + continue; + } + }; + if !content.contains(MANAGED_FILE_MARKER) { + report.errors.push(format!( + "unmanaged hook file blocks install for {phase} at {}; move it aside or run git smee install --force", + hook_path.display() + )); + continue; + } + report + .ok + .push(format!("managed wrapper is installed for {phase}")); + if !content.contains(&expected_config) { + report.warnings.push(format!( + "stale managed wrapper for {phase}: expected config path {}; run git smee install", + expected_config + )); + } + if let Some(expected_exe) = &expected_exe { + let expected_exe = expected_exe.to_string_lossy().to_string(); + if !content.contains(&expected_exe) { + report.warnings.push(format!( + "stale managed wrapper for {phase}: expected executable {expected_exe}; run git smee install" + )); + } + } + } + + finish_doctor_report(report) +} + +fn finish_doctor_report(mut report: DoctorReport) -> DoctorReport { + report.status = if !report.errors.is_empty() { + DoctorStatus::Error + } else if !report.warnings.is_empty() { + DoctorStatus::Warning + } else { + DoctorStatus::Ok + }; + report +} + +fn print_doctor_report(report: &DoctorReport) { + println!("git-smee doctor: {:?}", report.status); + if let Some(root) = &report.repository_root { + println!("repository root: {root}"); + } + if let Some(hooks_dir) = &report.hooks_dir { + println!("hooks directory: {hooks_dir}"); + } + println!("config path: {}", report.config_path); + print_doctor_section("ok", &report.ok); + print_doctor_section("warnings", &report.warnings); + print_doctor_section("errors", &report.errors); +} + +fn print_doctor_section(name: &str, items: &[String]) { + println!("{name}:"); + if items.is_empty() { + println!(" - none"); + } else { + for item in items { + println!(" - {item}"); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn finish_doctor_report_preserves_ok_when_no_findings() { + let report = finish_doctor_report(DoctorReport { + status: DoctorStatus::Error, + repository_root: None, + hooks_dir: None, + config_path: ".git-smee.toml".to_string(), + ok: vec!["inside a Git repository".to_string()], + warnings: Vec::new(), + errors: Vec::new(), + }); + + assert!(matches!(report.status, DoctorStatus::Ok)); + } + + #[test] + fn finish_doctor_report_prefers_errors_over_warnings() { + let report = finish_doctor_report(DoctorReport { + status: DoctorStatus::Ok, + repository_root: None, + hooks_dir: None, + config_path: ".git-smee.toml".to_string(), + ok: Vec::new(), + warnings: vec!["stale wrapper".to_string()], + errors: vec!["missing wrapper".to_string()], + }); + + assert!(matches!(report.status, DoctorStatus::Error)); + } +} diff --git a/crates/git-smee-cli/src/main.rs b/crates/git-smee-cli/src/main.rs index 0591880..8158ed7 100644 --- a/crates/git-smee-cli/src/main.rs +++ b/crates/git-smee-cli/src/main.rs @@ -1,7 +1,6 @@ use std::{ env, ffi::OsStr, - fs, io::{self, IsTerminal, Read}, path::{Component, Path, PathBuf}, str::FromStr, @@ -12,11 +11,11 @@ use git_smee_core::{ DEFAULT_CONFIG_FILE_NAME, SmeeConfig, config::{self, LifeCyclePhase}, executor, - installer::{self, HookInstaller, HookScriptOptions, MANAGED_FILE_MARKER}, + installer::{self, HookInstaller, HookScriptOptions}, repository, }; -use serde::Serialize; +mod doctor; mod status; const DEFAULT_MAX_HOOK_STDIN_BYTES: u64 = 10 * 1024 * 1024; @@ -199,225 +198,11 @@ fn run() -> Result<(), Box> { } Ok(()) } - Command::Doctor { json } => run_doctor(&config_path, json), + Command::Doctor { json } => doctor::run_doctor(&config_path, json), Command::Status { json } => status::run_status(&config_path, json), } } -#[derive(Debug, Serialize)] -struct DoctorReport { - status: DoctorStatus, - repository_root: Option, - hooks_dir: Option, - config_path: String, - ok: Vec, - warnings: Vec, - errors: Vec, -} - -#[derive(Debug, Serialize)] -#[serde(rename_all = "lowercase")] -enum DoctorStatus { - Ok, - Warning, - Error, -} - -fn run_doctor(config_path: &Path, json: bool) -> Result<(), Box> { - let report = build_doctor_report(config_path); - if json { - println!("{}", serde_json::to_string_pretty(&report)?); - } else { - print_doctor_report(&report); - } - if report.errors.is_empty() { - Ok(()) - } else { - Err("doctor found repository setup errors".into()) - } -} - -fn build_doctor_report(config_path: &Path) -> DoctorReport { - let mut report = DoctorReport { - status: DoctorStatus::Ok, - repository_root: None, - hooks_dir: None, - config_path: config_path.display().to_string(), - ok: Vec::new(), - warnings: Vec::new(), - errors: Vec::new(), - }; - - let repository_root = match repository::find_git_root() { - Ok(root) => { - report.ok.push("inside a Git repository".to_string()); - report.repository_root = Some(root.display().to_string()); - root - } - Err(error) => { - report.errors.push(format!( - "not inside a Git repository; run git smee doctor from a repository ({error})" - )); - return finish_doctor_report(report); - } - }; - - let hooks_dir = match repository::resolve_git_path( - &repository_root, - installer::FileSystemHookInstaller::HOOKS_GIT_PATH_KEY, - ) { - Ok(path) => { - report.hooks_dir = Some(path.display().to_string()); - if path.exists() && path.is_dir() { - report - .ok - .push(format!("hooks directory exists at {}", path.display())); - } else if path.exists() { - report.errors.push(format!( - "effective hooks path is not a directory: {}; fix core.hooksPath or remove the file", - path.display() - )); - } else { - report.warnings.push(format!( - "hooks directory does not exist yet at {}; run git smee install to create it", - path.display() - )); - } - path - } - Err(error) => { - report.errors.push(format!( - "could not resolve effective hooks directory; check git core.hooksPath ({error})" - )); - return finish_doctor_report(report); - } - }; - - let config = match read_config_file(config_path) { - Ok(config) => { - report - .ok - .push(format!("config parses from {}", config_path.display())); - if config.hooks.is_empty() { - report.errors.push( - "configuration contains no hooks; add at least one [[hook-name]] entry" - .to_string(), - ); - } else { - report.ok.push(format!( - "{} configured hook phase(s) are valid", - config.hooks.len() - )); - } - config - } - Err(error) => { - report.errors.push(format!( - "config problem at {}: {error}; run git smee init or fix the TOML file", - config_path.display() - )); - return finish_doctor_report(report); - } - }; - - let expected_config_path = normalize_config_path_for_hook_script(config_path, &repository_root) - .unwrap_or_else(|_| config_path.to_path_buf()); - let expected_exe = env::current_exe().ok(); - let expected_config = expected_config_path.to_string_lossy().to_string(); - - let mut phases: Vec<_> = config.hooks.keys().copied().collect(); - phases.sort_by_key(|phase| phase.as_str()); - for phase in phases { - let hook_path = hooks_dir.join(phase.as_str()); - if !hook_path.exists() { - report.errors.push(format!( - "missing managed wrapper for {phase} at {}; run git smee install", - hook_path.display() - )); - continue; - } - if !hook_path.is_file() { - report.errors.push(format!( - "hook path for {phase} is not a regular file: {}; remove it or fix core.hooksPath", - hook_path.display() - )); - continue; - } - let content = match fs::read_to_string(&hook_path) { - Ok(content) => content, - Err(error) => { - report.errors.push(format!( - "cannot read hook wrapper for {phase} at {}: {error}", - hook_path.display() - )); - continue; - } - }; - if !content.contains(MANAGED_FILE_MARKER) { - report.errors.push(format!( - "unmanaged hook file blocks install for {phase} at {}; move it aside or run git smee install --force", - hook_path.display() - )); - continue; - } - report - .ok - .push(format!("managed wrapper is installed for {phase}")); - if !content.contains(&expected_config) { - report.warnings.push(format!( - "stale managed wrapper for {phase}: expected config path {}; run git smee install", - expected_config - )); - } - if let Some(expected_exe) = &expected_exe - && !content.contains(&expected_exe.to_string_lossy().to_string()) - { - report.warnings.push(format!( - "stale managed wrapper for {phase}: expected executable {}; run git smee install", - expected_exe.display() - )); - } - } - - finish_doctor_report(report) -} - -fn finish_doctor_report(mut report: DoctorReport) -> DoctorReport { - report.status = if !report.errors.is_empty() { - DoctorStatus::Error - } else if !report.warnings.is_empty() { - DoctorStatus::Warning - } else { - DoctorStatus::Ok - }; - report -} - -fn print_doctor_report(report: &DoctorReport) { - println!("git-smee doctor: {:?}", report.status); - if let Some(root) = &report.repository_root { - println!("repository root: {root}"); - } - if let Some(hooks_dir) = &report.hooks_dir { - println!("hooks directory: {hooks_dir}"); - } - println!("config path: {}", report.config_path); - print_doctor_section("ok", &report.ok); - print_doctor_section("warnings", &report.warnings); - print_doctor_section("errors", &report.errors); -} - -fn print_doctor_section(name: &str, items: &[String]) { - println!("{name}:"); - if items.is_empty() { - println!(" - none"); - } else { - for item in items { - println!(" - {item}"); - } - } -} - fn resolve_config_path(cli_config: Option, invocation_dir: &Path) -> PathBuf { if let Some(path) = cli_config { return normalize_user_config_path(path, invocation_dir); From 9864df655d0563c3fee2c6a7d6b9678b4fb350f8 Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Tue, 23 Jun 2026 06:33:13 +0000 Subject: [PATCH 2/2] fix(cli): align doctor managed hook detection --- crates/git-smee-cli/src/doctor.rs | 18 +++++++++++++---- crates/git-smee-cli/tests/cli_integration.rs | 21 ++++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/crates/git-smee-cli/src/doctor.rs b/crates/git-smee-cli/src/doctor.rs index 273e00f..61f0154 100644 --- a/crates/git-smee-cli/src/doctor.rs +++ b/crates/git-smee-cli/src/doctor.rs @@ -1,6 +1,6 @@ use std::{env, fs, path::Path}; -use git_smee_core::{installer, installer::MANAGED_FILE_MARKER, repository}; +use git_smee_core::{installer, repository}; use serde::Serialize; use crate::{normalize_config_path_for_hook_script, read_config_file}; @@ -144,8 +144,8 @@ fn build_doctor_report(config_path: &Path) -> DoctorReport { )); continue; } - let content = match fs::read_to_string(&hook_path) { - Ok(content) => content, + let is_managed = match installer::has_managed_header(&hook_path) { + Ok(is_managed) => is_managed, Err(error) => { report.errors.push(format!( "cannot read hook wrapper for {phase} at {}: {error}", @@ -154,13 +154,23 @@ fn build_doctor_report(config_path: &Path) -> DoctorReport { continue; } }; - if !content.contains(MANAGED_FILE_MARKER) { + if !is_managed { report.errors.push(format!( "unmanaged hook file blocks install for {phase} at {}; move it aside or run git smee install --force", hook_path.display() )); continue; } + let content = match fs::read_to_string(&hook_path) { + Ok(content) => content, + Err(error) => { + report.errors.push(format!( + "cannot read hook wrapper for {phase} at {}: {error}", + hook_path.display() + )); + continue; + } + }; report .ok .push(format!("managed wrapper is installed for {phase}")); diff --git a/crates/git-smee-cli/tests/cli_integration.rs b/crates/git-smee-cli/tests/cli_integration.rs index 12ac6ff..e54686b 100644 --- a/crates/git-smee-cli/tests/cli_integration.rs +++ b/crates/git-smee-cli/tests/cli_integration.rs @@ -338,6 +338,27 @@ fn given_unmanaged_hook_when_doctor_then_collision_is_reported() { ); } +#[test] +fn given_marker_only_in_body_when_doctor_then_hook_is_unmanaged() { + let test_repo = common::TestRepo::default(); + let pre_commit = test_repo.path.join(".git/hooks/pre-commit"); + fs::write( + &pre_commit, + format!("#!/usr/bin/env sh\necho '{MANAGED_FILE_MARKER}'\n"), + ) + .unwrap(); + + Command::new(cargo::cargo_bin!("git-smee")) + .current_dir(&test_repo.path) + .arg("doctor") + .assert() + .failure() + .stdout( + predicate::str::contains("unmanaged hook file blocks install for pre-commit") + .and(predicate::str::contains("managed wrapper is installed for pre-commit").not()), + ); +} + #[test] fn given_custom_hooks_path_when_doctor_then_effective_path_is_reported() { let test_repo = common::TestRepo::default();