# swagger-cli Refactoring Plan: 5 System Weakness Remediations ## Epic 1: Output Sink Abstraction ### Problem Every command handler takes `robot: bool`, manually branches on it 29+ times, and contains its own formatting logic. The `output/` module is a hollow shell (human.rs is 8 lines). Adding new output formats (YAML is on the roadmap) requires touching every command. Output concerns are tangled into business logic. ### Solution Introduce an `OutputMode` enum and an `emit()` function that each command calls once with its typed data. Commands produce data; the output layer decides presentation. ### Design **New type in `src/output/mod.rs`:** ```rust pub enum OutputMode { Robot, Human, // Future: Yaml, Table, etc. } ``` **New `emit` function pattern:** Each command defines a `CommandOutput` trait impl or uses a generic `emit()` function. The `HumanDisplay` trait provides the human-readable rendering. ```rust pub trait HumanDisplay { fn display_human(&self, w: &mut dyn Write) -> std::io::Result<()>; } pub fn emit( data: &T, mode: OutputMode, command: &str, duration: Duration, ) { match mode { OutputMode::Robot => robot::robot_success(data, command, duration), OutputMode::Human => { let mut stdout = std::io::stdout().lock(); data.display_human(&mut stdout).expect("stdout write"); } } } ``` ### Implementation Steps 1. **Create `OutputMode` enum and `HumanDisplay` trait** in `src/output/mod.rs` 2. **Create `emit()` function** in `src/output/mod.rs` 3. **Convert `tags` command first** (simplest command, 168 lines) as proof-of-concept: - Impl `HumanDisplay` for `TagsOutput` - Replace `if robot_mode { ... } else { ... }` with `emit(&output, mode, "tags", duration)` - Remove `robot: bool` parameter from `execute()`, replace with `OutputMode` 4. **Propagate OutputMode through main.rs** -- replace `let robot = resolve_robot_mode(&cli)` with `let mode = resolve_output_mode(&cli)` returning `OutputMode` 5. **Convert remaining simple commands** one at a time (show, schemas, diff, search, cache_cmd, aliases, doctor, list) 6. **Convert fetch and sync** (most complex, last) 7. **Delete dead code** -- `robot: bool` parameters, inline formatting blocks ### TDD Plan **RED tests to write first (in `src/output/mod.rs` tests):** - `test_emit_robot_mode_produces_valid_json` -- call `emit()` with Robot mode, capture stdout, parse as `RobotEnvelope` - `test_emit_human_mode_produces_readable_text` -- call `emit()` with Human mode, verify no JSON in output - `test_human_display_tags_output` -- verify `TagsOutput::display_human()` produces table with correct headers **GREEN:** Implement `OutputMode`, `HumanDisplay`, `emit()` to pass tests. **REFACTOR:** Convert each command one at a time, running full test suite between each. ### Acceptance Criteria - Zero `if robot` / `if !robot` branches in command handlers - `output/human.rs` contains all human formatting logic (not 8 lines) - Adding a new output format requires zero changes to command handlers - All existing integration tests and golden tests pass unchanged ### Files Changed - `src/output/mod.rs` -- add `OutputMode`, `HumanDisplay`, `emit()` - `src/output/human.rs` -- move all human formatting here - `src/main.rs` -- `resolve_output_mode()` replaces `resolve_robot_mode()` - `src/cli/tags.rs` -- proof of concept conversion - `src/cli/show.rs`, `src/cli/schemas.rs`, `src/cli/diff.rs`, `src/cli/search.rs`, `src/cli/cache_cmd.rs`, `src/cli/aliases.rs`, `src/cli/doctor.rs`, `src/cli/list.rs` -- convert each - `src/cli/fetch.rs`, `src/cli/sync_cmd.rs` -- convert last (most complex) - `src/cli/mod.rs` -- no changes needed (Cli struct keeps `robot` flag, OutputMode resolves from it) ### Dependency Note This epic should be completed before Epic 2 (sync split) since sync_cmd conversion is part of this work. --- ## Epic 2: Split sync_cmd.rs ### Problem `sync_cmd.rs` is 1,843 lines containing 5+ distinct responsibilities: CLI args, diff computation, checkpoint/resume, per-host rate limiting, concurrent job pool, single-alias sync, batch sync, and output formatting. It's the most complex module and the hardest to reason about. ### Solution Split into a `sync/` directory with focused modules while preserving the existing public API (`execute()` function). ### Design **New directory: `src/cli/sync/`** | File | Responsibility | Lines (est.) | |------|---------------|-------------| | `mod.rs` | Re-exports, `Args` struct, `execute()` entry point | ~100 | | `types.rs` | All Serialize structs: `SyncOutput`, `AliasSyncResult`, `SyncAllOutput`, `ChangeSummary`, `ChangeDetails`, `EndpointKey`, `SchemaDiff`, `EndpointDiff` | ~100 | | `diff.rs` | `compute_diff()`, `endpoint_key()`, `endpoint_fingerprint()` | ~120 | | `checkpoint.rs` | `SyncCheckpoint`, `load_checkpoint()`, `save_checkpoint()`, `remove_checkpoint()` | ~60 | | `throttle.rs` | `PerHostThrottle`, `extract_host()` | ~50 | | `single.rs` | `sync_inner()`, `sync_one_alias()`, `sync_one_alias_inner()`, `output_no_changes()`, `output_changes()` | ~250 | | `batch.rs` | `sync_all_inner()` with concurrent stream logic | ~300 | ### Implementation Steps 1. **Create `src/cli/sync/` directory** 2. **Move types first** -- extract all `#[derive(Serialize)]` structs into `types.rs` 3. **Extract `diff.rs`** -- `compute_diff()`, `endpoint_key()`, `endpoint_fingerprint()`, `MAX_DETAIL_ITEMS` 4. **Extract `checkpoint.rs`** -- checkpoint load/save/remove + `CHECKPOINT_FILE` const 5. **Extract `throttle.rs`** -- `PerHostThrottle` and `extract_host()` 6. **Extract `single.rs`** -- single-alias sync logic and output helpers 7. **Extract `batch.rs`** -- `sync_all_inner()` concurrent execution 8. **Create `mod.rs`** -- `Args` struct, `execute()`, re-exports 9. **Update `src/cli/mod.rs`** -- replace `pub mod sync_cmd` with `pub mod sync` 10. **Update `src/main.rs`** -- `Commands::Sync` references ### TDD Plan **This is a pure refactor -- no new behavior.** The TDD approach is: - Run full test suite before starting: `cargo test` - After each extraction step, verify: `cargo test && cargo clippy --all-targets -- -D warnings` - No new tests needed (existing tests in `sync_cmd::tests` move to `sync/diff.rs` tests) **Specific test verification after each step:** - `test_diff_no_changes`, `test_diff_added_endpoint`, `test_diff_removed_endpoint`, `test_diff_modified_endpoint`, `test_diff_added_schema`, `test_diff_removed_schema`, `test_diff_endpoint_modified_by_params` -- move to `sync/diff.rs` - Integration tests (`tests/integration_test.rs`) that test `sync` command -- must pass unchanged ### Acceptance Criteria - `sync_cmd.rs` is deleted, replaced by `sync/` directory - No file in `sync/` exceeds 350 lines - All existing tests pass with zero modifications - `pub async fn execute()` signature unchanged - `cargo clippy --all-targets -- -D warnings` passes ### Files Changed - **Delete:** `src/cli/sync_cmd.rs` - **Create:** `src/cli/sync/mod.rs`, `types.rs`, `diff.rs`, `checkpoint.rs`, `throttle.rs`, `single.rs`, `batch.rs` - **Modify:** `src/cli/mod.rs` (module declaration), `src/main.rs` (import path if needed) ### Dependency Note Should be done after Epic 1 (output sink) since that will have already simplified the output formatting within sync. If done before Epic 1, the output formatting moves to `single.rs` and `batch.rs` and then needs to be re-extracted during Epic 1. --- ## Epic 3: spawn_blocking for Cache I/O ### Problem `CacheManager` uses `std::fs` operations and `fs2::FileExt` (flock) -- all blocking I/O. But command handlers are `async fn` running on tokio. Under `sync --all --jobs=4`, multiple concurrent tasks hit blocking cache I/O, potentially starving the tokio runtime. ### Solution Wrap cache operations in `tokio::task::spawn_blocking()`. Create async wrapper methods on a new `AsyncCacheManager` or add async variants directly to `CacheManager`. ### Design **Option A (recommended): Async wrapper struct** ```rust pub struct AsyncCache { inner: CacheManager, } impl AsyncCache { pub async fn load_index(&self, alias: &str) -> Result<(SpecIndex, CacheMetadata), SwaggerCliError> { let inner = self.inner.clone(); // CacheManager is just a PathBuf, cheap let alias = alias.to_string(); tokio::task::spawn_blocking(move || inner.load_index(&alias)) .await .map_err(|e| SwaggerCliError::Cache(format!("task join error: {e}")))? } // ... same pattern for write_cache, load_raw, list_aliases, etc. } ``` This preserves the existing sync `CacheManager` for tests and simple cases while providing an async-safe interface for the runtime. ### Implementation Steps 1. **Derive `Clone` for `CacheManager`** (it's just a `PathBuf` wrapper -- trivial) 2. **Create `AsyncCache` in `src/core/cache.rs`** with async wrappers for each public method: - `load_index()`, `load_raw()`, `write_cache()`, `list_aliases()`, `ensure_dirs()`, `alias_dir()`, `delete_alias_dir()`, `update_last_accessed()` 3. **Write tests for `AsyncCache`** verifying it produces identical results to sync `CacheManager` 4. **Convert command handlers** to use `AsyncCache` instead of `CacheManager`: - Start with `tags.rs` (simplest) - Then `list.rs`, `show.rs`, `search.rs`, `schemas.rs` - Then `sync_cmd.rs` / `sync/` (most critical -- this is where contention happens) - Then `fetch.rs`, `doctor.rs`, `cache_cmd.rs`, `aliases.rs` ### TDD Plan **RED tests first:** ```rust #[tokio::test] async fn test_async_cache_load_index_matches_sync() { // Setup: write a spec to cache using sync CacheManager // Act: load via AsyncCache // Assert: result matches sync CacheManager::load_index() } #[tokio::test] async fn test_async_cache_write_then_load_roundtrip() { // Write via AsyncCache, load via AsyncCache, verify integrity } #[tokio::test] async fn test_async_cache_concurrent_reads_no_panic() { // Spawn 10 concurrent load_index tasks on same alias // All should succeed (shared read lock) } ``` **GREEN:** Implement `AsyncCache` wrapper. **REFACTOR:** Convert command handlers one at a time. ### Acceptance Criteria - All cache I/O from async command handlers goes through `spawn_blocking` - Existing sync `CacheManager` preserved for non-async tests - `cargo test` passes (including lock_contention_test.rs) - No direct `std::fs` calls from async contexts in command handlers ### Files Changed - `src/core/cache.rs` -- add `Clone` derive, add `AsyncCache` struct - All `src/cli/*.rs` files -- replace `CacheManager::new()` with `AsyncCache::new()` ### Dependency Note Independent of Epics 1 and 2. Can be done in parallel. --- ## Epic 4: Property Tests for Search Engine and Parser ### Problem Property tests currently cover only 4 trivial properties (hash determinism, JSON roundtrip, index ordering, hash format). The search engine (722 LOC with scoring, tokenization, Unicode snippet handling) and the OpenAPI parser/indexer (659 LOC) have zero property test coverage. ### Solution Add targeted property tests using proptest for the search engine and format detection/parsing. ### Design **New test file: `tests/property_search_test.rs`** Property tests for search engine: 1. **Score monotonicity:** adding a matching field to an endpoint should never decrease its score 2. **Deterministic ordering:** same query + same index = same result order, always 3. **Limit respected:** result count <= opts.limit for any query 4. **Coverage boost property:** matching N/N terms scores >= matching 1/N terms 5. **Case insensitivity:** lowercased query on lowercased data = same results as mixed case 6. **Empty query safety:** any whitespace-only string returns empty 7. **Unicode safety:** search never panics on arbitrary Unicode input (including emoji, RTL, zero-width chars) 8. **Snippet bounds:** snippet length never exceeds 50 chars + ellipsis markers **New test file: `tests/property_indexer_test.rs`** Property tests for indexer: 1. **Format detection idempotent:** detect_format(bytes, hint, ct) called twice = same result 2. **JSON sniffing correct:** bytes starting with `{` or `[` always detected as JSON 3. **Normalize roundtrip:** normalize_to_json(bytes, Json) preserves semantic content 4. **build_index never panics on valid OpenAPI:** generate random valid-ish OpenAPI structures, verify build_index produces a valid SpecIndex 5. **Index endpoint count matches paths:** number of IndexedEndpoints = sum of methods across all paths 6. **Content hash deterministic:** same bytes -> same hash, every time ### Implementation Steps 1. **Create `tests/property_search_test.rs`** with proptest strategies for generating `SpecIndex` and `SearchOptions` 2. **Implement search property tests** (8 tests above) 3. **Create `tests/property_indexer_test.rs`** with proptest strategies for generating OpenAPI-like JSON 4. **Implement indexer property tests** (6 tests above) 5. **Run with extended case count** to verify: `cargo test -- --ignored` or `PROPTEST_CASES=1000 cargo test` ### TDD Plan These ARE the tests. Write them, then verify they pass against the existing implementation. If any fail, that's a real bug to fix. **Proptest strategy for SpecIndex generation:** ```rust fn arb_indexed_endpoint() -> impl Strategy { ( "/[a-z]{1,5}(/[a-z]{1,5}){0,3}", // path prop_oneof!["GET", "POST", "PUT", "DELETE", "PATCH"], // method proptest::option::of("[A-Za-z ]{1,50}"), // summary proptest::option::of("[A-Za-z ]{1,100}"), // description ).prop_map(|(path, method, summary, description)| { IndexedEndpoint { path, method, summary, description, /* ... defaults ... */ } }) } fn arb_spec_index() -> impl Strategy { proptest::collection::vec(arb_indexed_endpoint(), 0..50) .prop_map(|endpoints| SpecIndex { endpoints, /* ... */ }) } ``` ### Acceptance Criteria - 14 new property tests (8 search + 6 indexer) - All pass with default proptest case count (256) - No new dependencies (proptest already in dev-deps) - Tests catch at least one real edge case (likely Unicode snippet handling) ### Files Changed - **Create:** `tests/property_search_test.rs`, `tests/property_indexer_test.rs` - **Possibly modify:** `src/core/search.rs` if property tests reveal bugs ### Dependency Note Fully independent. Can be done in parallel with all other epics. --- ## Epic 5: Shared Command Pipeline ### Problem Every command handler independently: captures `Instant::now()`, creates `CacheManager::new(cache_dir())`, loads index with `cm.load_index(alias)`, does work, formats output, passes duration. This is ~15-20 lines of identical scaffolding per command. Adding cross-cutting concerns (telemetry, logging, caching headers) requires touching every command. ### Solution Create a `CommandContext` struct that encapsulates the common setup, and a `run_command()` helper that handles timing and output. ### Design ```rust /// Shared context created once per command invocation. pub struct CommandContext { pub cache: AsyncCache, // or CacheManager pub mode: OutputMode, pub start: Instant, pub network_policy: NetworkPolicy, pub config_override: Option, } impl CommandContext { pub fn new(mode: OutputMode, network_flag: &str, config_override: Option<&Path>) -> Result { Ok(Self { cache: AsyncCache::new(cache_dir()), mode, start: Instant::now(), network_policy: resolve_policy(network_flag)?, config_override: config_override.map(PathBuf::from), }) } pub fn load_index(&self, alias: &str) -> Result<(SpecIndex, CacheMetadata), SwaggerCliError> { self.cache.load_index(alias) } pub fn elapsed(&self) -> Duration { self.start.elapsed() } } ``` **Usage in commands:** ```rust pub async fn execute(args: &Args, ctx: &CommandContext) -> Result<(), SwaggerCliError> { let (index, meta) = ctx.load_index(&args.alias)?; let output = build_output(&index); emit(&output, ctx.mode, "tags", ctx.elapsed()); Ok(()) } ``` ### Implementation Steps 1. **Define `CommandContext` struct** in `src/cli/context.rs` 2. **Write tests** for `CommandContext::new()` and helper methods 3. **Update `main.rs`** to create `CommandContext` and pass to commands 4. **Convert `tags` first** (simplest) as proof of concept 5. **Convert all other commands** to accept `&CommandContext` 6. **Remove duplicated `CacheManager::new(cache_dir())` + `Instant::now()` from each handler** ### TDD Plan **RED tests first:** ```rust #[test] fn test_command_context_creates_valid_cache() { let ctx = CommandContext::new(OutputMode::Robot, "auto", None).unwrap(); // cache dir should be the platform default assert!(ctx.cache.cache_dir().exists() || true); // dir may not exist in test } #[test] fn test_command_context_elapsed_increases() { let ctx = CommandContext::new(OutputMode::Human, "auto", None).unwrap(); std::thread::sleep(Duration::from_millis(10)); assert!(ctx.elapsed().as_millis() >= 10); } #[test] fn test_command_context_offline_policy() { let ctx = CommandContext::new(OutputMode::Robot, "offline", None).unwrap(); assert_eq!(ctx.network_policy, NetworkPolicy::Offline); } ``` ### Acceptance Criteria - `CommandContext` created once in `main.rs`, passed to all commands - No command handler creates its own `CacheManager` or `Instant` - Each command's `execute()` signature includes `&CommandContext` - All tests pass unchanged ### Files Changed - **Create:** `src/cli/context.rs` - **Modify:** `src/cli/mod.rs` (add `pub mod context`) - **Modify:** `src/main.rs` (create CommandContext, pass to dispatch) - **Modify:** All `src/cli/*.rs` execute functions (accept `&CommandContext`) ### Dependency Note Depends on Epic 1 (OutputMode) and Epic 3 (AsyncCache). Should be done last. --- ## Execution Order ``` Epic 4 (property tests) ─────────────────────────────────────┐ Epic 3 (spawn_blocking) ─────────────────────────────────────┤ Epic 1 (output sink) ──> Epic 2 (split sync) ──> Epic 5 (pipeline) ``` Epics 3 and 4 are fully independent and can run in parallel with everything. Epic 1 should precede Epic 2 (simplifies sync output formatting before split). Epic 5 depends on Epics 1 and 3 (needs OutputMode and AsyncCache).