test: add MockRuntime and unit tests for command modules

[dean0x]

Summary

• Add a shared MockRuntime test double (src/orchestration/mock.rs) behind #[cfg(test)] with queued FIFO responses, call recording, and assertion helpers
• Refactor logs.rs, stop.rs, list.rs to extract testable inner functions
• Add 26 new unit tests across 6 modules (mock, logs, stop, list, cache, run)
• Add serial_test dev-dependency for smoke tests touching shared state

Closes #30

Test plan

• cargo test — 339 tests pass (up from 313)
• cargo clippy --all-targets — clean (only pre-existing warnings)
• cargo fmt -- --check — clean
• MockRuntime self-tests: default responses, queued responses, call recording
• logs: no container, returns output, line count, follow mode, error propagation
• stop: already stopped/failed, graceful, force, no container, tolerates missing, propagates errors
• list: filter active, filter all, empty input, JSON output, plain output
• cache: list empty, clear volumes, clear images, GC dry run
• run: interactive smoke, detached smoke (with #[serial])

[dean0x]

BLOCKING: Mock discards arguments

The run and create methods record an empty vec![] for args, discarding both ContainerConfig and command. This makes it impossible to use assert_called_with to verify the correct image, volumes, env vars, or command were passed.

Fix: Record at minimum the image and command:

async fn run(&self, config: &ContainerConfig, command: &[String]) -> MinoResult<String> {
    let mut args = vec![config.image.clone()];
    args.extend(command.iter().cloned());
    self.record("run", args);
    self.take_string("run", "mock-container-id")
}

(Tests review, B-1)

[dean0x]

BLOCKING: volume_create discards labels

The volume_create method records only the volume name but ignores the labels HashMap entirely. Labels are critical to cache state management. Tests that exercise cache creation flows cannot verify that correct labels were set.

Fix: Serialize labels into the args recording:

async fn volume_create(&self, name: &str, labels: &HashMap<String, String>) -> MinoResult<()> {
    let mut args = vec![name.to_string()];
    let mut sorted_labels: Vec<_> = labels.iter().collect();
    sorted_labels.sort_by_key(|(k, _)| *k);
    for (k, v) in sorted_labels {
        args.push(format!("{}={}", k, v));
    }
    self.record("volume_create", args);
    self.take_unit("volume_create")
}

(Tests review, B-2)

[dean0x]

BLOCKING: Vec::remove(0) is O(n) -- use VecDeque for FIFO

The take_response method uses queue.remove(0) which requires shifting all remaining elements. This is semantically a FIFO dequeue and should use VecDeque for O(1) performance and idiomatic correctness.

Fix:

use std::collections::VecDeque;

// In MockRuntime struct:
responses: Mutex<HashMap<String, VecDeque<MinoResult<MockResponse>>>>,

// In take_response:
fn take_response(&self, method: &str) -> Option<MinoResult<MockResponse>> {
    let mut responses = self.responses.lock().unwrap();
    let queue = responses.get_mut(method)?;
    queue.pop_front()
}

// In on():
.or_default()
.push_back(response);

(Performance & Rust review, M-1)

[dean0x]

BLOCKING: Missing #[serial] on env var tests

The resolve_layer_names_config_layers test calls std::env::remove_var("MINO_LAYERS") but lacks #[serial] annotation. Environment variables are process-global shared mutable state. Concurrent test execution can create data races and test flakiness in CI.

Since the PR introduced serial_test as a dependency and uses #[serial] on the smoke tests, this test should also be serialized for consistency.

Fix:

#[test]
#[serial]
fn resolve_layer_names_config_layers() {
    // ...
}

(Tests & Rust review, B-4/M-2)

[dean0x]

BLOCKING: Silent error discard should be logged

The extracted stop_container function uses let _ = runtime.remove(container_id).await; to silently discard errors from container removal. While the original code also did this, this extraction into a standalone testable function is the right time to add logging for the discarded error -- consistent with how run_interactive handles this at line 351-357.

Fix:

if let Err(e) = runtime.remove(container_id).await {
    tracing::debug!("Failed to remove container {}: {}", container_id, e);
}

This ensures there's a diagnostic trail if container removal fails due to timeout, permissions, or other issues.

(Rust review, M-3)

[dean0x]

Should-Fix: Repetitive take_ helper pattern (Complexity M-1)*

The nine take_* methods share an identical 4-arm match structure that only differs in the variant destructured and the default value returned. This is 79 lines of near-identical boilerplate. Adding new MockResponse variants in the future requires writing yet another copy-paste helper.

Suggested approach: Use a generic helper with a conversion closure to reduce per-type boilerplate to one line:

fn take<T>(
    &self,
    method: &str,
    default: T,
    extract: fn(MockResponse) -> Option<T>,
) -> MinoResult<T> {
    match self.take_response(method) {
        Some(Ok(resp)) => match extract(resp) {
            Some(val) => Ok(val),
            None => panic\!("wrong MockResponse variant for '{}'", method),
        },
        None => Ok(default),
        Some(Err(e)) => Err(e),
    }
}

Then each helper becomes:

fn take_bool(&self, method: &str, default: bool) -> MinoResult<bool> {
    self.take(method, default, |r| match r { MockResponse::Bool(b) => Some(b), _ => None })
}

(Complexity review)

[dean0x]

Should-Fix: Duplicate status guard (Consistency SF-2)

The extracted stop_container function contains an early-return guard for non-Running/Starting sessions (lines 75-80), but execute already has its own guard (lines 22-35) that checks the same condition. The stop_container guard is redundant since execute will never call it with a Stopped/Failed session.

Recommendation: Remove the guard from stop_container and update the doc comment, since the only caller already prevents that case. This improves clarity about which layer is responsible for the status check.

(Consistency review, SF-2)

[dean0x]

Should-Fix: Disable unnecessary default features on serial_test (Dependencies)

The serial_test = "3.4" uses default features which include async and logging. The async feature brings in futures-executor which is unnecessary because this project uses #[tokio::test] for async test execution. Tokio handles the async runtime; serial_test's async feature provides its own executor which is never used here.

Fix:

serial_test = { version = "3.4", default-features = false }

The #[serial] macro works with #[tokio::test] without the async feature. This eliminates futures-executor as an unnecessary transitive dependency (10 new crates added; removing one helps minimize build overhead).

(Dependencies review)

[dean0x]

Summary: PR Review Comments

Blocking Issues Fixed

All 5 blocking issues have been posted as inline comments:

1. B-1 (src/orchestration/mock.rs:219) - run and create mock methods discard arguments
2. B-2 (src/orchestration/mock.rs:299) - volume_create discards labels
3. M-1 (src/orchestration/mock.rs:123) - Vec::remove(0) O(n) -- use VecDeque
4. M-2/B-4 (src/cli/commands/run/mod.rs:607) - Missing #[serial] on env var tests
5. M-3 (src/cli/commands/stop.rs:100) - Silent error discard should be logged

Should-Fix Issues (Ready for Review)

All 3 should-fix items have been posted as inline comments:

1. Complexity M-1 (src/orchestration/mock.rs:126) - Repetitive take_* helpers
2. Consistency SF-2 (src/cli/commands/stop.rs:75) - Duplicate status guard in stop_container
3. Dependencies SF-1 (Cargo.toml:51) - Unnecessary default features on serial_test

Pre-Existing Issues (Informational Only)

These issues exist in the codebase but are not introduced by this PR:

Architecture:

• PE-1: ContainerRuntime trait has large surface area (20 methods) -- consider splitting into ContainerRuntime, ImageManager, VolumeManager in future PR
• PE-2: Command modules mix UI/presentation with business logic (pre-existing design)
• PE-3: std::sync::Mutex in MockRuntime (acceptable for test code with short lock hold times)

Regression:

• PE-1: stop_container returns Ok(true) when container_id is None (pre-existing semantic)

Security:

• PE-1: Container removal silently ignores errors (pre-existing pattern)
• PE-2: Session files stored as plain JSON on disk (pre-existing architecture)

Tests:

• PE-1: No unit tests for run_interactive error paths (cache finalization errors, non-zero exit codes)
• PE-2: No unit tests for run_detached background finalization path
• PE-3: No test for print_table output formatting

Performance:

• PE-1/PE-2: Sequential volume operations in clear_artifacts and gc_caches (could be parallelized with join_all)

Test Coverage Summary

• Total tests: 339 unit + 13 integration
• All passing with 100% success rate
• New MockRuntime enables unit-testable boundaries across 5 command modules

Recommendation

Status: Request Changes

Path Forward:

1. Fix 5 blocking issues (B-1, B-2, M-1, M-2/B-4, M-3)
2. Address 3 should-fix items (Complexity M-1, Consistency SF-2, Dependencies SF-1)
3. Consider pre-existing improvements in follow-up PRs

---

Review Scores:

• Architecture: 8/10
• Complexity: 8/10
• Consistency: 8/10
• Dependencies: 8/10
• Performance: 9/10
• Regression: 9/10
• Rust: 8/10
• Security: 9/10
• Tests: 8/10

Generated by Claude Code Review Automation