fix: ANSI-aware table column alignment using console::pad_str

[dean0x]

Summary

Fixes ANSI-aware table column alignment across cache, list, and home volume tables. Replaces Rust's format specifiers with console::pad_str() to properly handle invisible ANSI escape codes in styled text.

Changes

• src/cli/commands/cache.rs: Updated print_cache_table() and print_home_table()

  • VOLUME, ECOSYSTEM, STATE, SIZE, CREATED columns now use pad_str()
  • Width constants extracted for maintainability
  • Home table columns: VOLUME, PROJECT, CREATED with truncation on overflow

• src/cli/commands/list.rs: Updated print_table()

  • NAME, STATUS, STARTED, PROJECT columns now use pad_str()
  • Styled text (bold, colors) properly handled for alignment
  • Width constants for consistency

Why This Matters

Rust's {:<width} format specifier counts all bytes including invisible ANSI escape codes. When text is styled (bold, colored), these codes inflate the byte count, causing columns to misalign. The console crate's pad_str() counts only visible characters, fixing alignment.

Before: Columns shifted right when status/names were styled
After: Columns align properly regardless of ANSI styling

Testing

• Manual verification: Run mino cache list and mino list with styled output
• Existing tests continue to pass (no behavior change, only formatting)
• Table readability improved with correct column alignment

Related

Complements the two-phase bootstrap and update check infrastructure.

[dean0x]

Duplicated shell command resolution (reported by: Architecture, Rust, Complexity, Consistency, Regression - 95% confidence)

Lines 263-272 duplicate the shell command logic from lines 200-208 in the same function. Both compute the same conditional: if using layers, use /bin/zsh, else use config.session.shell.

This creates a DRY violation that's a maintenance hazard — if one location is updated without the other, the command used for iptables wrapping (line 200) could diverge from the command used in the two-phase exec phase (line 263), causing subtle debugging issues.

Suggested fix: Compute the shell command once before the iptables wrapping, then reuse:

let shell_command = if is_shell_mode {
    if using_layers {
        vec!["/bin/zsh".to_string()]
    } else {
        vec![config.session.shell.clone()]
    }
} else {
    vec![]
};

let command = if is_shell_mode {
    shell_command.clone()
} else {
    args.command.clone()
};

// Then iptables wrapping only affects the container creation, not the shell_command stored in RunContext

[dean0x]

Security issue: CAP_NET_ADMIN not dropped in exec phase (Regression - 90% confidence)

The two-phase bootstrap applies iptables rules in phase 1 via generate_iptables_wrapper (line 511). However, the exec phase at line 562 calls the shell directly without wrapping with capsh --drop=cap_net_admin.

When NetworkMode::Allow is active, this is a regression: users with the container shell can run iptables -F to flush the firewall rules and bypass network restrictions. The original start_attached flow wraps the command with capsh to prevent this (line 213).

Suggested fix: Wrap the exec'd shell command with capsh if in network-allow mode:

let exec_command = if let NetworkMode::Allow(_) = ctx.network_mode {
    vec![
        "capsh".to_string(),
        "--drop=cap_net_admin".to_string(),
        "--".to_string(),
        "-c".to_string(),
        format!("exec {}", ctx.shell_command.join(" ")),
    ]
} else {
    ctx.shell_command.clone()
};

[dean0x]

Resource leak: child process not waited after kill (Rust - 85% confidence)

After calling child.kill().await on line 119 (timeout path) and line 149 (marker found path), the code does not call child.wait().await to reap the child process. On Unix, kill() sends SIGKILL but the process becomes a zombie until its exit status is waited for. Relying on the Child destructor to reap is fragile.

Suggested fix: Add child.wait().await after each child.kill().await:

// Timeout path (line 119):
let _ = child.kill().await;
let _ = child.wait().await;

// Marker found path (line 149):
let _ = child.kill().await;
let _ = child.wait().await;

[dean0x]

Unused parameter in function signature (reported by: Rust, Architecture, Performance, Consistency, Complexity - 92% confidence)

The _cache_session parameter is prefixed with underscore to suppress unused warnings, indicating it's not actually used inside run_interactive_shell. Cache finalization happens in the caller run_interactive (lines 425-427), so passing this parameter adds noise to the API and could mislead readers into thinking cache handling was delegated.

Suggested fix: Remove the parameter from both the function signature and the call site:

// Function signature:
async fn run_interactive_shell(ctx: &mut RunContext<'_>) -> MinoResult<i32> {

// Call site in run_interactive:
run_interactive_shell(ctx).await?

[dean0x]

Race condition: non-atomic state file write (Security - 82% confidence)

The fire-and-forget tokio::spawn block (lines 236-245) reads, mutates, and writes the version state file concurrently with the main thread. If two mino sessions start simultaneously (common in CI or multi-terminal workflows), both may read the same stale state before either writes back. The save_state_to function uses tokio::fs::write which is not atomic — a crash or concurrent write mid-operation could corrupt the file.

While this is low-risk for a local advisory-only state file, the pattern should be correct for production robustness.

Suggested fix: Use atomic write pattern (write to temp, then rename):

async fn save_state_to(path: &Path, state: &VersionState) {
    // ... existing dir creation ...
    let tmp = path.with_extension("json.tmp");
    if let Err(e) = tokio::fs::write(&tmp, json).await {
        warn!("Failed to write version state: {}", e);
        return;
    }
    if let Err(e) = tokio::fs::rename(&tmp, path).await {
        warn!("Failed to rename version state: {}", e);
    }
}

[dean0x]

Table styling inconsistency (Consistency - 90% confidence)

The headers in this file ("VOLUME", "ECOSYSTEM", etc.) are plain strings, but list.rs wraps headers in style(...).bold() before calling pad_str. Both files were modified in this PR to adopt pad_str, but the styling convention diverged instead of being unified.

Since bold headers (as in list.rs) provide better visual hierarchy, apply bold styling consistently across both files:

pad_str(&style("VOLUME").bold().to_string(), W_VOLUME, Alignment::Left, None),
pad_str(&style("ECOSYSTEM").bold().to_string(), W_ECO, Alignment::Left, None),
// ... etc for all headers in both functions

[dean0x]

Security: Predictable log file path in /tmp (Security - 85% confidence)

The bootstrap output is redirected to /tmp/mino-bootstrap.log, which is a predictable path. Inside a rootless container the risk is limited, but the path is not collision-proof. If multiple containers somehow shared /tmp (unlikely with Podman but possible with custom mounts), one container's bootstrap output could be read or overwritten.

Install output may contain URLs, version info, or error messages with sensitive path details.

Suggested fix: Use mktemp for a unique filename or write to the user's home directory:

# Option 1: Use mktemp
INSTALL_REDIRECT="$(mktemp /tmp/mino-bootstrap-XXXXXX.log)"

# Option 2: Use home directory (safer)
INSTALL_REDIRECT="$HOME/.mino-bootstrap.log"

[dean0x]

Code Review Summary: feat/two-phase-bootstrap

This branch implements a well-designed two-phase bootstrap architecture for interactive shell mode. The refactoring correctly preserves existing behavior for explicit commands while eliminating terminal pollution during environment setup. Below is a consolidated overview of review findings.

Review Coverage

The PR was reviewed across 8 dimensions: Security, Architecture, Performance, Complexity, Consistency, Regression, Tests, and Rust Code Quality. All 503 existing tests pass.

Blocking Issues (7 High-Confidence Findings)

All 7 items below appear as inline comments and require changes before merge:

1. Duplicated shell command resolution (95% confidence)

   • Lines 200-208 and 263-272 in run/mod.rs duplicate identical logic
   • Risk: future changes could cause the two paths to diverge
   • Impact: maintenance hazard, subtle debugging confusion

2. Security regression: CAP_NET_ADMIN not dropped in exec phase (90% confidence)

   • Two-phase flow wraps phase 1 with iptables but does not drop CAP_NET_ADMIN in the exec'd shell
   • Risk: users in --network-allow mode can run iptables -F to bypass firewall rules
   • This is a regression from the original start_attached behavior

3. Zombie process not reaped (85% confidence)

   • follow_until_marker calls child.kill().await but not child.wait().await
   • Risk: process becomes zombie until destructor runs (fragile)

4. Unused parameter _cache_session (92% confidence)

   • Parameter is never used; cache finalization happens in the caller
   • Creates API noise and misleads readers about delegated responsibility

5. Non-atomic version state file write (82% confidence)

   • Fire-and-forget tokio::spawn reads, mutates, and writes state concurrently
   • Two simultaneous sessions could corrupt the version state JSON file
   • Pattern should use atomic write (temp file + rename)

6. Table header styling inconsistency (90% confidence)

   • cache.rs headers are plain strings; list.rs headers are bold
   • Both files were modified in this PR and should be unified

7. Bootstrap log path security (85% confidence)

   • Changed from /dev/stderr to /tmp/mino-bootstrap.log (predictable path)
   • Should use mktemp or write to $HOME/.mino-bootstrap.log

Additional Findings (60-79% Confidence)

These are noted for context but do not block merge if architectural decision is intentional:

• Double check_for_update call (85%): Both startup and exit trigger file reads
• Test coverage gaps (90%, 85%): follow_until_marker lacks unit tests; run_interactive_shell error paths untested
• Hardcoded bootstrap timeout (70%): 300 seconds not configurable
• Fire-and-forget error reporting (72%): Background update check silently swallows errors
• iptables wrapper confusion (80%): ctx.command becomes stale data in shell path (document or restructure)
• Shell_command ownership (82%): Stored as Vec<String> instead of slice, causing clones in tests

Recommendations

Before Merge:

1. Fix duplicated shell command (lines 263-272) — consolidate to single computation
2. Address security regression — wrap exec'd shell with capsh --drop=cap_net_admin when NetworkMode::Allow is active
3. Reap zombie processes — add child.wait().await after each child.kill().await
4. Remove unused _cache_session parameter
5. Fix version state file writes — use atomic write pattern
6. Unify table styling — apply bold headers consistently in both files
7. Improve bootstrap log path — use mktemp or $HOME/.mino-bootstrap.log

Post-Merge Improvements (non-blocking):

• Extract execute() version-check and credential-gathering phases into helpers (246 lines → 200 target)
• Add unit tests for follow_until_marker using real child processes
• Add error path tests for run_interactive_shell (start_detached failure, logs_follow_until timeout)
• Document fire-and-forget update check behavior and first-run experience
• Refactor RunContext if it accumulates further mode-specific fields

Architecture Assessment

The two-phase architecture is sound: phase 1 creates a container with sleep infinity and applies iptables wrapping, then phase 2 monitors logs for the "Bootstrap complete." marker before exec-ing the interactive shell. The ContainerRuntime trait extensions (start_detached, logs_follow_until) follow established patterns. The fire-and-forget update check refactoring eliminates blocking HTTP latency from the startup path, a genuine UX improvement.

All inline comments above include suggested fixes with code examples.

---

Generated by Claude Code automated review | Review branch: feat/two-phase-bootstrap | Base: main | Confidence threshold: ≥80%