feat: Wave 0 test audit — comprehensive rskim-search tests#111
Closed
leesharon wants to merge 14 commits into
Closed
feat: Wave 0 test audit — comprehensive rskim-search tests#111leesharon wants to merge 14 commits into
leesharon wants to merge 14 commits into
Conversation
leesharon
commented
Mar 31, 2026
leesharon
commented
- feat: rskim-search crate foundation — types, traits, CLI skeleton (Release v0.5.0: Markdown Support #3)
- fix: remove unused serde_json dependency from rskim-search
- fix: address self-review issues
- test: Wave 0 audit — comprehensive rskim-search tests
Introduce the rskim-search crate as the search and indexing foundation for the 3-layer code search system. All types and traits are I/O-free and follow the rskim-core patterns exactly. - crates/rskim-search: new crate with Cargo.toml, lib.rs, types.rs, traits.rs - types.rs: FileId, SearchField, MatchSpan, LineRange, TemporalFlags, SearchQuery (builder pattern), SearchResult, IndexStats, FileTable (I/O-free normalize), SearchError (thiserror), Result<T> alias + 10 unit tests covering all types - traits.rs: SearchLayer, LayerBuilder, FieldClassifier trait definitions with full doc comments - CLI: add `skim search` stub subcommand with full clap arg definitions (--ast, --limit, --blast-radius, --hot, --cold, --risky, --build, etc.) - Workspace: add rskim-search member, wire into rskim dependency - main.rs: register --ast and --limit as value-consuming flags - completions.rs: register search::command() for accurate tab completions - cli_search.rs: 4 integration tests (help, -h, stub with query, stub empty)
serde_json was declared as a dependency but never imported or used anywhere in the crate source. Removes unnecessary build-time overhead.
- Add #![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] to error_conversion.rs and serialization_roundtrip.rs so crate-level deny lints do not break cargo clippy --tests - Replace 3.14 score literal with 2.75 to avoid clippy::approx_constant
Add 87 new tests across 6 test files (5 new + 1 extended): - error_conversion.rs: 89 lines (error type conversions) - file_table_edge_cases.rs: 180 lines (new) (FileTable corner cases) - serialization_roundtrip.rs: 273 lines (serde round-trip integrity) - trait_object_safety.rs: 68 lines (new) (trait object handling) - types_edge_cases.rs: 270 lines (new) (SearchQuery/types validation) - cli_search.rs: extended with exit code, help text validation Implementation changes supporting tests: - FileTable custom Serialize/Deserialize (emit paths vec only) - SearchQuery #[must_use] attribute - serde_json added to rskim-search dev-dependencies - Cargo.lock updated with rskim-search dependency in rskim crate All 1700+ workspace tests passing. Co-Authored-By: Claude <noreply@anthropic.com>
…t comments, doc drift - search.rs: replace manual print_help() body with command().print_help() so help text auto-tracks clap definition without drift (DRY violation fixed) - rskim/Cargo.toml: add INTENTIONAL comment on rskim-search dep to document it is scaffolding wired up in Wave 1, not dead weight - rskim-search/Cargo.toml: add readme/keywords/categories to match rskim-core metadata pattern; add explanatory comments to [lints.clippy] section - main.rs: update is_flag_with_value() docstring to explicitly cover both Args struct flags and subcommand-only flags (--ast, --limit, etc.) Co-Authored-By: Claude <noreply@anthropic.com>
…asts, fast-path Five issues from batch-1 audit of types.rs: - Deserialization now runs normalize() on each path before building the ids map, so a table serialized with ./src/main.rs round-trips idempotently (re-registering the same unnormalized path after deser returns the existing FileId rather than creating a duplicate). - Replace silent `as usize` / `as u64` casts in lookup() with usize::try_from(...).ok()? so truncation becomes a None return rather than wrong-file access. Add compile_error! to reject 32-bit targets where usize < u64 would make the casts lossy. - Swap clone order in register(): ids takes the clone, paths takes the move — avoids one unnecessary PathBuf allocation per new registration. - Fast-path in normalize(): skip Vec<Component> allocation entirely when the path contains no CurDir or ParentDir components (the common case). - Add two serialization_roundtrip tests covering the deser-normalize invariant: unnormalized-paths-idempotent-after-roundtrip and serialized-form-uses-normalized-paths. Co-Authored-By: Claude <noreply@anthropic.com>
…build/update stub coverage - file_table_edge_cases: add test_normalize_absolute_over_root documenting that /a/../../b produces /../b (I/O-free normalizer cannot clamp at root) - cli_search: add regression tests for --build, --rebuild, --update flags confirming each routes through the not-yet-implemented stub path Co-Authored-By: Claude <noreply@anthropic.com>
types.rs was 577 lines, exceeding the 400-line project convention.
Split into four focused modules under types/:
- file_table.rs — FileId, FileTable, custom serde impls, normalize(),
64-bit compile_error gate
- query.rs — SearchQuery (#[must_use]), TemporalFlags, SearchField
- result.rs — SearchResult, MatchSpan, LineRange, IndexStats
- error.rs — SearchError, Result alias
mod.rs barrel re-exports all types so lib.rs pub use is unchanged.
Inline unit tests moved into their respective sub-modules.
All 91 tests pass; clippy --workspace -D warnings clean.
Co-Authored-By: Claude <noreply@anthropic.com>
command().print_help() renders the command name from clap's
Command::new("search"), which omits the parent binary. Override with
.name("skim search") so help output shows "Usage: skim search".
Simplifier reverted to manual println pattern; restoring clap delegation since eliminating help text duplication was the explicit review finding (flagged by 4 reviewers). Uses let _ = to handle io::Result.
…xHashMap - Add register_within(path, root) that rejects paths escaping project root (leading .. or absolute after normalization) - Add MAX_FILE_TABLE_ENTRIES (10M) size limit on FileTable deserialization to prevent OOM from malicious/corrupted index files - Switch FileTable from HashMap to FxHashMap (rustc-hash) for faster path lookups — zero-cost change, already a transitive dependency - 7 new tests covering all three fixes
…m-search from rskim - Move rustc-hash = "1.1" from rskim-search inline declaration into [workspace.dependencies] so all crates follow the same pattern. - Remove the rskim-search dependency from the rskim CLI crate; no code in rskim imports it (search.rs is a pure stub). The dep will be re-added when Wave 1 wires up real search logic. Fixes batch-2 issues: cargo:rustc_hash_workspace, cargo:unused_rskim_search_dep Co-Authored-By: Claude <noreply@anthropic.com>
…ble normalization - register_within now verifies containment by joining root+path, re-normalizing, and checking starts_with(normalized_root); previously _root was unused so any relative path passed regardless of root - Extract register_normalized(PathBuf) private method so both register() and register_within() normalize exactly once instead of twice - Deserialization now returns Err on duplicate normalized paths (e.g., ["./src/main.rs","src/main.rs"]) rather than silently overwriting the ids map and breaking the paths.len() == ids.len() invariant - Add test_file_table_deserialization_rejects_at_limit_plus_one covering the actual rejection branch of the 10M-entry size limit (was previously untested) - Add test_file_table_deserialization_rejects_duplicate_normalized_paths - Add register_within tests: single-parent escape, subdirectory escape, root-itself acceptance; document that relative paths without ".." do resolve inside root when joined Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.