Feat/code search - #14
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR raises Rust and loopctl requirements, changes configuration mappings, replaces session state with per-run todo state, adds shared search and CodeSearch functionality, strengthens Python and input validation, and updates related tools and tests. ChangesConfiguration and provider updates
Tool infrastructure
Validation and tool behavior
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CodeSearchTool
participant SearchInput
participant RegexCache
participant FileWalker
participant Output
Caller->>CodeSearchTool: submit search request
CodeSearchTool->>SearchInput: parse filters and limits
CodeSearchTool->>RegexCache: compile or retrieve pattern
CodeSearchTool->>FileWalker: scan eligible files
FileWalker-->>CodeSearchTool: bounded matches
CodeSearchTool->>Output: render or spill results
Output-->>Caller: search response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/dch-tools/src/read.rs (1)
159-175: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRead at most the configured file-size cap.
A file can grow after the metadata check.
tokio::fs::readthen reads and allocates the full new size before Lines 160 and 172 reject it.Use a bounded reader with a limit of
MAX_FILE_SIZE_BYTES + 1for both image and text paths. Reject the result when the extra byte is present.Proposed direction
- let bytes = tokio::fs::read(&full_path).await?; + let bytes = read_bounded(&full_path, MAX_FILE_SIZE_BYTES + 1).await?; if let Some(too_large) = too_large_if_over(bytes.len() as u64) { return Ok(too_large); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/dch-tools/src/read.rs` around lines 159 - 175, Update the image and text read paths in the surrounding read function to use a bounded reader limited to MAX_FILE_SIZE_BYTES + 1 instead of tokio::fs::read, then reject results containing the extra byte while preserving the existing too_large_if_over handling and content processing for files within the cap.
🧹 Nitpick comments (5)
crates/dch-tools/src/input.rs (1)
28-38: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider treating an explicit JSON
nullas absent.
get_usizeandget_u64rejectValue::NullwithInvalidInput. Models frequently emit"context_lines": nullfor an unset optional field. That input now fails the whole tool call instead of applying the default.If you keep the strict behavior, the current tests already cover it. If you prefer tolerance, map
NulltoOk(None).♻️ Optional: treat null as absent
pub fn get_usize(input: &Value, key: &str) -> Result<Option<usize>, ToolError> { match input.get(key) { - None => Ok(None), + None | Some(Value::Null) => Ok(None), Some(Value::Number(n)) => nAlso applies to: 67-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/dch-tools/src/input.rs` around lines 28 - 38, Update get_usize and get_u64 to treat an explicit Value::Null like a missing optional field by returning Ok(None), while preserving current numeric parsing and invalid-type errors.crates/dch-tools/src/search.rs (1)
289-295: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCheck the file size before sniffing for binary content.
likely_binaryopens the file and reads up to 8 KiB.file_too_largeonly reads metadata. The current order sniffs an oversized file that is then discarded.Swapping the two predicates makes the cheap check run first and removes one open plus one 8 KiB read per oversized file.
♻️ Proposed reorder
- if walk::likely_binary(path) || walk::file_too_large(path) { + if walk::file_too_large(path) || walk::likely_binary(path) { continue; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/dch-tools/src/search.rs` around lines 289 - 295, In the file filtering condition around entry.path(), evaluate walk::file_too_large(path) before walk::likely_binary(path). Preserve the existing continue behavior while ensuring oversized files are rejected before binary-content sniffing.crates/dch-tools/src/code_search.rs (1)
222-227: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared relative-path computation.
These six lines are identical to
scan_fileincrates/dch-tools/src/grep.rs(lines 214-218), including the"unknown"fallback.crates/dch-tools/src/search.rsdocuments itself as the owner of everything both tools share, and theMatch::filedoc already describes this exact fallback rule.Move the computation into
search.rsas a small helper and call it from both tools. That keeps the documented contract and the implementation in one place.♻️ Proposed helper in crates/dch-tools/src/search.rs
/// Compute a `Match::file` value for `file_path` relative to `base_path`. /// /// Falls back to the full path, then to `"unknown"` for non-UTF8 paths. #[must_use] pub fn relative_file(file_path: &Path, base_path: &Path) -> &str { file_path .strip_prefix(base_path) .ok() .and_then(|p| p.to_str()) .unwrap_or_else(|| file_path.to_str().unwrap_or("unknown")) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/dch-tools/src/code_search.rs` around lines 222 - 227, Extract the duplicated relative-path logic into a public `relative_file` helper in `search.rs`, preserving the existing `strip_prefix` behavior and full-path/`"unknown"` fallbacks. Replace the local computation in `code_search` and `scan_file` in `grep.rs` with calls to this shared helper.crates/dch-tools/src/output.rs (1)
10-18: 🧹 Nitpick | 🔵 TrivialSpilled files accumulate without a cleanup owner.
The module documents that cleanup belongs to the runner, and that no shutdown hook exists today. Every oversized search result therefore leaves a persistent file in the OS temp dir. A long-lived agent process with many large searches grows this set without bound.
Two low-cost mitigations are available while the shutdown hook is missing:
- Delete the previous spill file for the same tool and session before writing a new one.
- Sweep
dch-spill-*directories older than a fixed age at runner startup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/dch-tools/src/output.rs` around lines 10 - 18, Add bounded spill cleanup in the output handling flow: remove the previous spill file for the same tool and session before creating a replacement, and add a runner-startup sweep for stale dch-spill-* directories older than a fixed age. Anchor the changes to session_temp_dir and the runner initialization path, preserving graceful inline truncation on cleanup or spill failures.crates/dch-tools/src/regex_cache.rs (1)
164-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo cache tests cannot fail. Both tests assert only that a returned regex matches its own pattern. A cache miss transparently recompiles, so that assertion holds whether or not the cache behaved as the test name claims. Neither test inspects
cache_size(), which the module already provides.
crates/dch-tools/src/regex_cache.rs#L164-L174:cache_lru_reorders_on_accessnever compiles_secondor_third, so no eviction pressure exists and no reordering is exercised. Compile all three patterns, then re-accessfirst, then compile enough new patterns to exceedCACHE_CAPACITY, and assert thatfirstis still resident while an untouched entry is gone.crates/dch-tools/src/regex_cache.rs#L150-L162:cache_evicts_at_capacityomits the leadingclear_cache()and asserts only that the newest pattern resolves. Callclear_cache()first, then assertcache_size() == CACHE_CAPACITYafter filling past the limit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/dch-tools/src/regex_cache.rs` around lines 164 - 174, The regex cache tests do not verify eviction or LRU behavior. In crates/dch-tools/src/regex_cache.rs:164-174, update cache_lru_reorders_on_access to compile all three initial patterns, re-access first, then compile enough additional patterns to exceed CACHE_CAPACITY and use cache_size() or residency checks to confirm first remains while an untouched entry is evicted. In crates/dch-tools/src/regex_cache.rs:150-162, add clear_cache() before filling the cache and assert cache_size() == CACHE_CAPACITY after exceeding the limit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/dch-config/src/lib.rs`:
- Around line 300-306: Enforce the documented 0..=100 range for
compact_threshold in to_session_config(), clamping or rejecting values above 100
before constructing loopctl::config::SessionConfig. Add a regression test that
covers an out-of-range value such as 101 or 255 and verifies the chosen
behavior.
In `@crates/dch-tools/src/bash.rs`:
- Line 337: Update the schema near the "required": [] definition to retain the
empty universal requirement while adding an anyOf constraint requiring either
command or operation. Adjust the associated schema test to verify that inputs
containing command or operation pass, while an empty object fails validation
before call_inner handles it.
- Around line 399-401: Validate the parsed timeout in the flow containing
get_u64 before applying min(MAX_TIMEOUT_SECS), returning InvalidInput when the
value is explicitly zero while preserving the default for an omitted value.
Apply the same validation in the other timeout-handling path around the
referenced logic, and add a regression test beside the negative-timeout test
covering zero.
In `@crates/dch-tools/src/code_search.rs`:
- Around line 280-283: Update the result header formatting in the code-search
function to use singular “match” when matches.len() is 1 and plural “matches”
otherwise; adjust the corresponding assertion in the relevant test to expect
“Found 1 match”.
- Around line 303-310: Update the context-mode rendering branch in the
code-search command to sort matches by file path and then line number before
iterating over them. Preserve the existing output formatting while ensuring
deterministic ordering, and leave the succinct-mode BTreeMap grouping unchanged.
In `@crates/dch-tools/src/linter.rs`:
- Around line 207-288: The python_indent_check validation currently loses
PythonScanner quote state between lines, so multiline triple-quoted strings are
incorrectly checked as code. Update python_indent_check and the supporting
scanner flow to preserve string state across the complete source while still
skipping string/comment content and tracking real delimiters; add a regression
test covering mixed indentation and delimiters inside a multiline string,
including Write, Edit, and MultiEdit validation paths.
In `@crates/dch-tools/src/search.rs`:
- Line 227: The max_results parsing in parse_input must reject or clamp an
explicit zero so it honors CodeSearch’s minimum of 1; apply the same treatment
to max_matches in grep’s input parsing before constructing per_file_cap. Update
crates/dch-tools/src/search.rs:227-227 and crates/dch-tools/src/grep.rs:167-167;
negative values should continue producing an error, while valid positive values
retain their existing behavior.
In `@crates/dch-tools/src/util.rs`:
- Around line 65-69: Update the URL scheme detection in is_url to compare HTTP
and HTTPS schemes with eq_ignore_ascii_case, ensuring uppercase or mixed-case
forms are rejected with the existing WebFetch guidance while preserving behavior
for file://, ftp://, bare paths, and empty strings.
---
Outside diff comments:
In `@crates/dch-tools/src/read.rs`:
- Around line 159-175: Update the image and text read paths in the surrounding
read function to use a bounded reader limited to MAX_FILE_SIZE_BYTES + 1 instead
of tokio::fs::read, then reject results containing the extra byte while
preserving the existing too_large_if_over handling and content processing for
files within the cap.
---
Nitpick comments:
In `@crates/dch-tools/src/code_search.rs`:
- Around line 222-227: Extract the duplicated relative-path logic into a public
`relative_file` helper in `search.rs`, preserving the existing `strip_prefix`
behavior and full-path/`"unknown"` fallbacks. Replace the local computation in
`code_search` and `scan_file` in `grep.rs` with calls to this shared helper.
In `@crates/dch-tools/src/input.rs`:
- Around line 28-38: Update get_usize and get_u64 to treat an explicit
Value::Null like a missing optional field by returning Ok(None), while
preserving current numeric parsing and invalid-type errors.
In `@crates/dch-tools/src/output.rs`:
- Around line 10-18: Add bounded spill cleanup in the output handling flow:
remove the previous spill file for the same tool and session before creating a
replacement, and add a runner-startup sweep for stale dch-spill-* directories
older than a fixed age. Anchor the changes to session_temp_dir and the runner
initialization path, preserving graceful inline truncation on cleanup or spill
failures.
In `@crates/dch-tools/src/regex_cache.rs`:
- Around line 164-174: The regex cache tests do not verify eviction or LRU
behavior. In crates/dch-tools/src/regex_cache.rs:164-174, update
cache_lru_reorders_on_access to compile all three initial patterns, re-access
first, then compile enough additional patterns to exceed CACHE_CAPACITY and use
cache_size() or residency checks to confirm first remains while an untouched
entry is evicted. In crates/dch-tools/src/regex_cache.rs:150-162, add
clear_cache() before filling the cache and assert cache_size() == CACHE_CAPACITY
after exceeding the limit.
In `@crates/dch-tools/src/search.rs`:
- Around line 289-295: In the file filtering condition around entry.path(),
evaluate walk::file_too_large(path) before walk::likely_binary(path). Preserve
the existing continue behavior while ensuring oversized files are rejected
before binary-content sniffing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9dc2358e-2167-46de-8c62-295c642714bf
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
.clippy.tomlCargo.tomlcrates/dch-config/Cargo.tomlcrates/dch-config/src/lib.rscrates/dch-loop/src/provider.rscrates/dch-tools/Cargo.tomlcrates/dch-tools/src/bash.rscrates/dch-tools/src/code_search.rscrates/dch-tools/src/context.rscrates/dch-tools/src/diff.rscrates/dch-tools/src/edit.rscrates/dch-tools/src/file_viewer.rscrates/dch-tools/src/glob.rscrates/dch-tools/src/grep.rscrates/dch-tools/src/input.rscrates/dch-tools/src/lib.rscrates/dch-tools/src/linter.rscrates/dch-tools/src/multi_edit.rscrates/dch-tools/src/output.rscrates/dch-tools/src/question.rscrates/dch-tools/src/read.rscrates/dch-tools/src/regex_cache.rscrates/dch-tools/src/registry.rscrates/dch-tools/src/runtime.rscrates/dch-tools/src/search.rscrates/dch-tools/src/state.rscrates/dch-tools/src/todo.rscrates/dch-tools/src/util.rscrates/dch-tools/src/walk.rscrates/dch-tools/src/write.rsrust-toolchain.tomlrustfmt.toml
💤 Files with no reviewable changes (5)
- crates/dch-config/Cargo.toml
- crates/dch-tools/src/runtime.rs
- rustfmt.toml
- rust-toolchain.toml
- crates/dch-tools/src/state.rs
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/dch-tools/src/search.rs (1)
204-216: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject wrong types instead of applying defaults.
A present non-string
pathbecomes".". A present non-booleancase_insensitivebecomesfalse. A malformed request can search the wrong directory or return a false no-match result. Parse present values with explicit type checks and returnToolError::InvalidInput.get_string_listalso drops non-string elements; reject them when the field contract requires strings. (raw.githubusercontent.com)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/dch-tools/src/search.rs` around lines 204 - 216, Update parse_input and its get_string_list helper to reject present values with incorrect types instead of applying defaults or silently dropping elements. Validate path as a string, case_insensitive as a boolean, and required string-list elements as strings; return ToolError::InvalidInput for each mismatch while preserving defaults only for absent fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/dch-tools/src/linter.rs`:
- Around line 327-332: Update update_triple_state to scan the line while
tracking escape characters, and only clear state when an unescaped matching
triple-quote delimiter is found; escaped delimiters such as \""" must leave the
string open. Add a regression test covering an escaped triple quote followed by
mixed whitespace inside the triple-quoted string.
In `@crates/dch-tools/src/search.rs`:
- Around line 229-231: Update the max_results parsing in the Grep search flow to
clamp user-provided values to the same hard maximum used by CodeSearch, while
preserving the existing default and minimum-of-one behavior. Add a regression
test covering an oversized max_results value and verify it is reduced to the cap
before SearchJob receives it.
- Around line 310-313: Update the file-reading flow around walk::file_too_large,
walk::likely_binary, and std::fs::read_to_string to open each file once and read
no more than walk::MAX_FILE_BYTES + 1 bytes. Reject files that produce the extra
byte before decoding the bytes as UTF-8, preserving the existing skip behavior
for oversized or unreadable files.
In `@crates/dch-tools/src/util.rs`:
- Around line 72-74: Update is_url to use safe substring access via get(..7) and
get(..8) with is_some_and instead of direct byte slicing, preserving the
existing HTTP/HTTPS checks without panicking on arbitrary UTF-8 input. Add a
test covering a non-ASCII path whose byte length crosses a prefix boundary.
---
Outside diff comments:
In `@crates/dch-tools/src/search.rs`:
- Around line 204-216: Update parse_input and its get_string_list helper to
reject present values with incorrect types instead of applying defaults or
silently dropping elements. Validate path as a string, case_insensitive as a
boolean, and required string-list elements as strings; return
ToolError::InvalidInput for each mismatch while preserving defaults only for
absent fields.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: df7d541a-3d1e-459e-bade-6ab09aee2069
📒 Files selected for processing (8)
crates/dch-config/src/lib.rscrates/dch-tools/src/bash.rscrates/dch-tools/src/code_search.rscrates/dch-tools/src/grep.rscrates/dch-tools/src/linter.rscrates/dch-tools/src/read.rscrates/dch-tools/src/search.rscrates/dch-tools/src/util.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/dch-tools/src/code_search.rs
- crates/dch-tools/src/read.rs
- crates/dch-tools/src/grep.rs
- crates/dch-config/src/lib.rs
- crates/dch-tools/src/bash.rs
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
1 participant
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.
No description provided.