feat(exec): add mino exec subcommand for running commands in sessions

[dean0x]

Summary

This implements mino exec — a new subcommand that executes a command inside a running mino session container with full exit code propagation.

Changes

Core Feature:

• ExecArgs CLI struct with optional session name and -- command separator
• Session resolution logic: looks up named session or defaults to most recent running session
• TTY detection for interactive shell usage (defaults to /bin/zsh)
• Exit code propagation through main.rs (MinoResult<ExitCode>)

Trait Extension:

• Added exec_in_container() method to ContainerRuntime trait
• Implementations for all 3 runtimes: NativePodmanRuntime, OrbStackRuntime, MockRuntime
• Uses podman exec -i (+ -t if TTY) with interactive stream handling

Error Handling:

• NoActiveSessions error with actionable hint: "Start a session with: mino run"
• Validates named sessions are in Running state before exec
• Returns container not found error if session lacks container_id

CLI Argument Parsing:

• mino exec — defaults to most recent running, shell /bin/zsh
• mino exec SESSION_NAME — exec into named session
• mino exec -- ls -la — run command in most recent session
• mino exec SESSION -- ls -la — run command in named session

Testing

18 new unit tests covering:

• CLI parsing (4 tests): no args, with session, with command, session+command
• Pure resolution logic (8 tests): find_running_session, validate_session_running behavior
• MockRuntime integration (6 tests): TTY forwarding, exit code propagation, error handling

All tests validate behavior rather than implementation details.

Breaking Changes

None. Exit code propagation in main() is transparent to existing commands (all return Ok(ExitCode::SUCCESS)).

Related Issues

Closes #42

[dean0x]

Return Type Inconsistency

This function returns MinoResult<ExitCode> while all 9 other command modules return MinoResult<()>. This breaks the uniform command interface established across the codebase.

Flagged by: Architecture (HIGH), Consistency (HIGH), Complexity (HIGH)

Recommendation: Change exec::execute() to return MinoResult<()> and call std::process::exit() internally for non-zero codes (same pattern run could adopt):

pub async fn execute(args: ExecArgs, config: &Config) -> MinoResult<()> {
    // ... existing logic ...
    let exit_code = exec_in_session(&session, &*runtime, &command, tty).await?;
    if exit_code != 0 {
        std::process::exit(exit_code as i32);
    }
    Ok(())
}

This aligns with the existing pattern where other commands (like run) handle exit code semantics internally and return Ok(()) to main.rs.

Claude Code Review

[dean0x]

S-1: Session Name Validation Missing

The session argument from ExecArgs (user-supplied CLI input) is passed directly to SessionManager::get(), which constructs a file path without validation. A user could supply path traversal strings like ../../../etc/passwd.

Flagged by: Security (MEDIUM)

This is a defense-in-depth violation. The codebase already has validate_layer_name() in src/layer/resolve.rs that prevents path traversal.

Recommendation: Add session name validation:

fn validate_session_name(name: &str) -> MinoResult<()> {
    if name.is_empty()
        || name.contains('/')
        || name.contains('\\')
        || name.contains("..")
        || name.contains('\0')
        || \!name.chars().all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_')
    {
        return Err(MinoError::User(format\!(
            "Invalid session name '{}': must contain only alphanumeric characters, hyphens, or underscores",
            name
        )));
    }
    Ok(())
}

Call this in resolve_session() or at the Session::load() boundary.

Claude Code Review

[dean0x]

B-1: Debug Formatting for User-Facing Status String

The validate_session_running function formats SessionStatus using {:?} (Debug) and lowercases it:

format!("{:?}", session.status).to_lowercase()

This couples user-facing error messages to Rust's Debug trait output. If SessionStatus adds multi-word variants (e.g., ShuttingDown), the output becomes "shuttingdown" which is not readable.

Flagged by: Rust (HIGH), Complexity (MEDIUM)

Recommendation: Implement Display for SessionStatus in src/session/state.rs:

impl std::fmt::Display for SessionStatus {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        match self {
            Self::Starting => write!(f, "starting"),
            Self::Running => write!(f, "running"),
            Self::Stopped => write!(f, "stopped"),
            Self::Failed => write!(f, "failed"),
        }
    }
}

Then use {} formatting in exec.rs instead of {:?}.to_lowercase().

Claude Code Review

[dean0x]

B-2: Exit Code Truncation Lacks Logging

The exit code is masked and cast to u8:

Ok(ExitCode::from((exit_code & 0xFF) as u8))

When exec_interactive returns -1 (process killed by signal, no exit code), this silently converts to 255. While POSIX-correct, the distinction between "exited with 255" and "killed by signal" is lost.

Flagged by: Regression (HIGH), Security (MEDIUM)

Recommendation: Add explicit handling and logging:

let exit_code = exec_in_session(&session, &*runtime, &command, tty).await?;
if exit_code < 0 {
    debug!("Container process terminated by signal (raw exit code: {})", exit_code);
}
Ok(ExitCode::from((exit_code & 0xFF) as u8))

This documents the behavior and aids debugging when a process is killed by signal.

Claude Code Review

[dean0x]

SF-3: Dispatch Boilerplate - Inconsistent Match Arm Formatting

The dispatch match now has two distinct patterns in a single block:

Commands::Exec(args) => mino::cli::commands::exec(args, &config).await,
Commands::Run(args) => { mino::cli::commands::run(args, &config).await?; Ok(ExitCode::SUCCESS) }

The single-line { ...; Ok(ExitCode::SUCCESS) } pattern repeated 8 times is boilerplate that obscures the dispatch table.

Flagged by: Complexity (HIGH), Consistency (MEDIUM), Rust (HIGH)

Recommendation: Extract a helper to eliminate duplication:

async fn ok_success<F, Fut>(f: F) -> MinoResult<ExitCode>
where
    F: FnOnce() -> Fut,
    Fut: std::future::Future<Output = MinoResult<()>>,
{
    f().await?;
    Ok(ExitCode::SUCCESS)
}

// Then:
Commands::Run(args) => ok_success(mino::cli::commands::run(args, &config)).await,
Commands::Exec(args) => mino::cli::commands::exec(args, &config).await,

Alternatively, align with the note under issue SF-1 and have all commands return MinoResult<()>, handling exit codes internally (preferred long-term).

Claude Code Review

[dean0x]

SF-2: Missing Doc Comment on execute Function

The execute() function lacks a doc comment. Every other command module has:

/// Execute the {name} command
pub async fn execute(...) -> MinoResult<()>

Flagged by: Consistency (MEDIUM)

Recommendation: Add the doc comment:

/// Execute the exec command
pub async fn execute(args: ExecArgs, config: &Config) -> MinoResult<ExitCode> {

This maintains consistency with established conventions across all command modules.

Claude Code Review

[dean0x]

PR #42 Code Review Summary

Review Scores by Discipline

Reviewer	Score	Recommendation
Security	7/10	APPROVED_WITH_CONDITIONS
Architecture	7/10	CHANGES_REQUESTED
Performance	8/10	APPROVED
Complexity	8/10	APPROVED_WITH_CONDITIONS
Consistency	7/10	CHANGES_REQUESTED
Regression	8/10	APPROVED_WITH_CONDITIONS
Tests	8/10	APPROVED_WITH_CONDITIONS
Rust	8/10	APPROVED_WITH_CONDITIONS

Average Score: 7.6/10

---

Consolidated Issues Summary

BLOCKING / HIGH SEVERITY (5 issues)

1. Return Type Inconsistency (Architecture, Consistency, Complexity)

   • exec::execute() returns MinoResult<ExitCode> while all 9 other command modules return MinoResult<()>
   • Creates dual dispatch pattern in main.rs
   • Fix: Change to MinoResult<()> with internal std::process::exit() call

2. Session Name Validation Missing (Security)

   • User-supplied CLI input flows directly to file path without validation
   • Allows path traversal attacks (e.g., ../../../etc/passwd)
   • Fix: Add validate_session_name() check consistent with validate_layer_name() pattern

3. Debug Formatting for User-Facing Status (Rust)

   • Uses format!("{:?}", status).to_lowercase() in error messages
   • Couples user output to Debug trait, fragile with future enum variants
   • Fix: Implement Display for SessionStatus

4. Exit Code Truncation Lacks Logging (Regression, Security)

   • Silent conversion of -1 (signal) to 255 loses semantic meaning
   • Fix: Add debug log to document signal termination behavior

5. Dispatch Boilerplate in main.rs (Complexity, Rust)

   • Eight match arms repeat { cmd.await?; Ok(ExitCode::SUCCESS) } pattern
   • Fix: Extract helper function or migrate all commands to MinoResult<ExitCode>

SHOULD FIX (5 issues)

• Missing doc comment on execute() function (Consistency)
• Error variant inconsistency: Uses MinoError::User instead of structured SessionNotRunning variant (Architecture)
• UI framework bypass: Raw eprintln! instead of ui::step_info() (Consistency)
• OrbStack allocation overhead: Unnecessary Vec<String> → Vec<&str> conversion (Performance/Rust)
• Test coverage gap: No test for Failed session status validation (Tests)

INFORMATIONAL / PRE-EXISTING (6 issues)

• Session names in run --name also lack validation (pre-existing, same pattern as S-1)
• run command silently drops exit code (architectural gap, pre-existing)
• Redundant test pair: exec_delegates_to_runtime / exec_tty_false_forwarded
• File length warnings on args.rs, native_podman.rs, orbstack_runtime.rs (pre-existing)
• exec_interactive convention of -1 sentinel for signals (pre-existing, works correctly)
• Help output ordering change (low impact, reasonable)

---

Positive Observations

1. Excellent separation of concerns: Four pure functions (resolve_session, validate_session_running, find_running_session, exec_in_session) cleanly decompose the logic
2. Proper dependency injection: exec_in_session accepts &dyn ContainerRuntime for testability
3. Correct trait extension: exec_in_container added to all three runtimes (native, OrbStack, mock) consistently
4. Well-organized tests: 18 tests cleanly separated by category (parsing, pure functions, runtime integration)
5. Error handling: Uses established MinoError variants with proper .hint() entries
6. Mock infrastructure: MockRuntime::exec_in_container correctly records calls in FIFO order with sensible default

---

Merge Recommendation

CHANGES_REQUESTED - The PR has architectural merit and the exec command itself is well-designed. However, it introduces a pattern deviation (return type inconsistency) that will compound maintenance burden as more commands need exit code propagation.

Conditions for Merge

1. REQUIRED: Fix blocking return type inconsistency (SF-1) - Choose Option A (conform exec to existing pattern) or Option B (migrate all commands uniformly)
2. REQUIRED: Add session name validation to prevent path traversal (S-1)
3. REQUIRED: Implement Display for SessionStatus and use it instead of Debug formatting (B-1)
4. RECOMMENDED: Add debug log for signal termination (B-2)
5. RECOMMENDED: Add doc comment to execute() function (SF-2)
6. RECOMMENDED: Extract dispatch boilerplate helper or align with return type fix (SF-3)
7. NICE-TO-HAVE: Add integration test for exec error path, fix OrbStack allocations

---

Inline Comments

See 6 inline comments on the PR for specific line-level feedback.

---

Generated by Claude Code Review
2026-03-13T11:24:00Z