refactor: [#116] address code review feedback

Co-authored-by: josecelano <58816+josecelano@users.noreply.github.com>

Author: Copilot

packages/dependency-installer/tests/containers/helpers.rs

@@ -47,6 +47,13 @@ pub fn copy_file_to_container(container_id: &str, source_path: &Path, dest_path:
 /// # Returns
 ///
 /// The combined stdout and stderr output as a string
+///
+/// # Note
+///
+/// The output combines stderr and stdout because the CLI uses tracing which writes
+/// logs to stderr, while user-facing messages go to stdout. We need both for
+/// comprehensive test assertions. Stderr is placed first to maintain chronological
+/// order of log messages relative to output.
 pub fn exec_in_container(container_id: &str, command: &[&str]) -> String {
     let output = Command::new("docker")
         .arg("exec")
@@ -55,7 +62,7 @@ pub fn exec_in_container(container_id: &str, command: &[&str]) -> String {
         .output()
         .expect("Failed to execute docker exec command");
 
-    // Combine stdout and stderr to capture all output (logs go to stderr)
+    // Combine stderr (logs) and stdout (user messages) to capture all output
     let stdout = String::from_utf8_lossy(&output.stdout);
     let stderr = String::from_utf8_lossy(&output.stderr);
     format!("{}{}", stderr, stdout)
@@ -70,7 +77,12 @@ pub fn exec_in_container(container_id: &str, command: &[&str]) -> String {
 ///
 /// # Returns
 ///
-/// The exit code of the command
+/// The exit code of the command, or 1 if the process was terminated by a signal
+///
+/// # Note
+///
+/// If the process was terminated by a signal (returns None from code()), we return 1
+/// to indicate failure rather than 0, which would incorrectly suggest success.
 pub fn exec_in_container_with_exit_code(container_id: &str, command: &[&str]) -> i32 {
     let status = Command::new("docker")
         .arg("exec")
@@ -79,5 +91,6 @@ pub fn exec_in_container_with_exit_code(container_id: &str, command: &[&str]) ->
         .status()
         .expect("Failed to execute docker exec command");
 
-    status.code().unwrap_or(0)
+    // Return 1 (failure) if terminated by signal, otherwise use actual exit code
+    status.code().unwrap_or(1)
 }

packages/dependency-installer/tests/docker_check_command.rs

@@ -67,9 +67,11 @@ async fn test_check_specific_tool() {
     // Check a specific tool (OpenTofu)
     let output = container.exec(&["dependency-installer", "check", "--tool", "opentofu"]);
 
-    // The output contains the status line with the checkmark/X symbol
+    // The output contains "OpenTofu: not installed" in the status line
+    // We check for the plain text version since the ✗ symbol may not be present
+    // in all terminal environments or when output is redirected
     assert!(
-        output.contains("✗ OpenTofu: not installed") || output.contains("OpenTofu: not installed"),
+        output.contains("OpenTofu: not installed"),
         "Expected OpenTofu to be reported as not installed, got: {}",
         output
     );
@@ -149,16 +151,34 @@ async fn test_verbose_output() {
 ///
 /// This function assumes the binary was built before running tests.
 /// Run `cargo build --bin dependency-installer` before running these tests.
+///
+/// # Implementation Note
+///
+/// We use CARGO_MANIFEST_DIR and navigate up to the workspace root, then into
+/// the target directory. This works because:
+/// 1. CARGO_MANIFEST_DIR points to packages/dependency-installer
+/// 2. The workspace root is two directories up
+/// 3. The target directory is in the workspace root
+///
+/// Alternative approaches considered:
+/// - CARGO_TARGET_DIR: Not always set
+/// - OUT_DIR: Points to build script output, not target/debug
+/// - Searching for target dir: Too expensive
 fn get_binary_path() -> PathBuf {
-    // Get the workspace root directory
-    let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
-
-    // Navigate to workspace target directory
-    path.pop(); // packages
-    path.pop(); // repository root
-    path.push("target");
-    path.push("debug");
-    path.push("dependency-installer");
+    // Get the package manifest directory (packages/dependency-installer)
+    let manifest_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
+
+    // Navigate to workspace root (two levels up from packages/dependency-installer)
+    let workspace_root = manifest_dir
+        .parent() // packages/
+        .and_then(|p| p.parent()) // workspace root
+        .expect("Failed to find workspace root");
+
+    // Build path to the binary in target/debug
+    let path = workspace_root
+        .join("target")
+        .join("debug")
+        .join("dependency-installer");
 
     assert!(
         path.exists(),