refactor: deduplicate volume operations between runtimes

[dean0x]

Summary

Closes #24.

• Extracted 3 pub(crate) volume JSON parsing helpers into src/orchestration/mod.rs: parse_volume_labels, parse_volume_list_json, parse_volume_inspect_json (plus private volume_info_from_json)
• Refactored volume_list and volume_inspect in both native_podman.rs and orbstack_runtime.rs to delegate to shared helpers via super::
• Eliminated ~75 lines of duplicated JSON parsing logic across the two runtime implementations
• Added 20 unit tests covering all extracted helpers (edge cases: null labels, empty input, invalid JSON, prefix filtering, non-string label values)
• Simplified collect_disk_usage and volume_disk_usage with idiomatic iterator patterns

No behavioral changes. No trait or API changes. All callers continue using dyn ContainerRuntime unchanged.

Test plan

• cargo test — 310 unit + 13 integration tests pass
• cargo clippy — clean (2 pre-existing warnings in unmodified code)
• 20 new unit tests for extracted helpers
• Manual smoke test with mino cache list on Linux/macOS

[dean0x]

JSON Error Type Regression

The new helpers parse_volume_list_json and parse_volume_inspect_json convert serde_json::Error via .map_err(|e| MinoError::Internal(e.to_string())). This discards the structured error source chain and loses the MinoError::Json variant that is already defined with #[from].

The codebase has:

// In src/error.rs
Json(#[from] serde_json::Error)

This means you can use the ? operator to automatically convert the error. This is a MEDIUM-priority fix since centralizing JSON parsing is the right time to fix this across both call sites at once.

Suggested fix:

// Before (line 137):
let volumes: Vec<serde_json::Value> =
    serde_json::from_str(stdout).map_err(|e| MinoError::Internal(e.to_string()))?;

// After:
let volumes: Vec<serde_json::Value> = serde_json::from_str(stdout)?;

Apply the same change at line 175 in parse_volume_inspect_json.

This improves error diagnostics and makes error categorization more precise for callers that pattern-match on MinoError::Json.

[dean0x]

JSON Error Type at Second Call Site

Same issue as the comment at line 137: this line should use the MinoError::Json variant via the ? operator instead of .map_err(|e| MinoError::Internal(e.to_string())).

Suggested fix:

// Before:
let volumes: Vec<serde_json::Value> =
    serde_json::from_str(stdout).map_err(|e| MinoError::Internal(e.to_string()))?;

// After:
let volumes: Vec<serde_json::Value> = serde_json::from_str(stdout)?;

[dean0x]

Asymmetric Empty-Input Handling Between Sibling Functions

The parse_volume_list_json function has an explicit empty/whitespace guard (lines 131-133), but the sibling function parse_volume_inspect_json (starting at line 170) does not. Both functions parse the same kind of Podman JSON output.

If parse_volume_inspect_json receives empty/whitespace stdout, it will produce a MinoError::Internal from serde_json::from_str, while parse_volume_list_json gracefully returns an empty result. This internal inconsistency creates a subtle trap for future callers.

Suggested fix:
Add the same empty-input guard to parse_volume_inspect_json. This makes the two functions symmetric in their handling of edge cases and prevents silent errors from malformed Podman output.

[dean0x]

Silent Data Loss: unwrap_or_default() Masks Missing Volume Names

The line uses unwrap_or_default() to convert missing or non-string Name field to empty string. This empty name then fails the prefix check and is silently filtered out, hiding potentially malformed volume entries.

While Podman always includes Name, this pattern creates a silent data loss path. If Podman ever returns a volume with a missing or non-string Name, it would disappear with no warning.

Suggested fix:
Use filter_map with explicit type checking to make intent clear. This makes the contract explicit: only volumes with valid string names are processed. Missing names are filtered rather than coerced to empty strings.

[dean0x]

Code Review Summary - PR #40

Blocking Issues (MEDIUM priority - fix before merge):

1. JSON error mapping at lines 137 and 175: Both lines use .map_err(|e| MinoError::Internal(e.to_string())) instead of leveraging the existing MinoError::Json(#[from] serde_json::Error) variant. This discards structured error information. Use the ? operator instead (automatic conversion via #[from]).

2. Asymmetric empty-input handling: parse_volume_list_json (line 132) has an empty-input guard, but parse_volume_inspect_json (line 170) lacks it. Add the same if stdout.trim().is_empty() { return Ok(None); } guard to parse_volume_inspect_json for consistency.

3. Silent data loss via unwrap_or_default(): Line 142 uses vol["Name"].as_str().unwrap_or_default() which coerces missing/non-string names to empty strings, hiding malformed entries. Use filter_map with explicit type checking to make intent clear.

---

Should-Fix Issues (MEDIUM priority - consider for this PR):

• Redundant volume inspect calls: Both native_podman.rs and orbstack_runtime.rs still call volume inspect per-volume to fetch mountpoints, even though volume_list already parsed and stored them in VolumeInfo.mountpoint. This is an N+1 pattern. Since this PR extracted mountpoint parsing, using that field is now straightforward. This eliminates N subprocess calls per volume_disk_usage call.

• Test coverage gap: parse_volume_inspect_json tests lack assertion for created_at field. Add assertion for symmetry with the list parser tests.

---

Test Quality: 20 new unit tests added covering edge cases. 323 unit + 13 integration tests pass.

Overall Assessment: Excellent deduplication refactor. Blocking issues are straightforward to fix in src/orchestration/mod.rs.

---

Code review via Claude Code | 2026-03-11