feat: Native file watching and incremental analysis cache#168
Merged
andrewdavidmackenzie merged 8 commits intomasterfrom Mar 24, 2026
Merged
feat: Native file watching and incremental analysis cache#168andrewdavidmackenzie merged 8 commits intomasterfrom
andrewdavidmackenzie merged 8 commits intomasterfrom
Conversation
Implements reliable binary change detection using native file watching instead of relying on LSP client file watchers (which many IDEs don't support for paths outside source directories). ## Native File Watching (file_watcher.rs) - Uses `notify` crate (cross-platform: FSEvents/inotify/ReadDirectoryChangesW) - Watches target/debug/ for binary/library changes - Watches config files (jonesy.toml, Cargo.toml) - Debounced event processing (500ms) ## Incremental Analysis Cache (analysis_cache.rs) - Stored in target/jonesy/cache.json - Tracks binary modification times to skip unchanged targets - Detects workspace structure changes: - Adding/removing workspace members → full re-analysis - Adding/removing/changing binary paths → targeted re-analysis - Adding/removing/changing library paths → targeted re-analysis ## Documentation - Updated docs/lsp.md with file watching and caching details - Comprehensive tests for all monitored file types Closes #166 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds native file watching (using the notify crate) and an on-disk incremental analysis cache. New modules: Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
The inline allow check was using std::env::current_dir() to resolve relative file paths, which doesn't work in the LSP context where the working directory may not be the workspace root. Now the workspace root is passed through the call chain: - analyze_macho -> collect_crate_code_points -> filter_allowed_causes -> check_inline_allow This fixes inline allow comments not working in the LSP. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add workspace_root parameter to check_inline_allow and filter_allowed_causes calls. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jonesy/src/analysis_cache.rs`:
- Around line 23-24: The mtime field currently stores seconds (pub mtime: u64)
and get_mtime() uses Duration::as_secs(), losing subsecond precision; change
mtime to hold subsecond resolution (e.g. change pub mtime: u64 to pub mtime:
u128 or pub mtime: u64 representing milliseconds), update the code that sets
mtime to use Duration::as_millis() (or as_nanos() if you choose nanosecond
precision) instead of as_secs(), and update get_mtime() and any comparisons in
target_needs_analysis() to compare the new millisecond/nanosecond value so
rebuilds within the same second are detected. Ensure any
serialization/deserialization or persisted formats that reference mtime are
updated accordingly.
In `@jonesy/src/file_watcher.rs`:
- Around line 127-142: The startup logic currently skips calling watcher.watch
when config.target_dir or a config_file doesn't exist, so the watcher misses
first-time creations; remove the exists() guards and always register watches
(for files, watch their parent dir via config_file.parent()) using
watcher.watch(..., RecursiveMode::NonRecursive), and ignore/non-fatal-handle
errors as you already do (e.g., map_err for target_dir or let _ = for
config_files) so the watcher will receive creation events even if the target
directory or jonesy.toml is absent at startup.
- Around line 176-191: The is_binary_or_library function currently misses
Windows executables; update its extension handling so that Some("exe") (and
consider case-insensitive matches) is treated as a binary (return true) like the
other library extensions in the match for extension, ensuring
target/debug/<name>.exe triggers BinaryChanged; locate and modify the match arm
in is_binary_or_library to include "exe" (or normalize extension to lowercase
before matching) so Windows outputs are recognized.
- Around line 115-121: The file_watcher notify callback currently uses
tx_clone.blocking_send in two places (sending WatchEvent instances and
WatchEvent::Error), which contradicts the "Non-blocking send - drop events if
channel is full" comment; replace both blocking_send calls with
tx_clone.try_send and handle the Result by ignoring the Err(Full) case (dropping
overflow events) and logging/propagating other errors if needed, locating these
changes in the notify callback where WatchEvent and WatchEvent::Error are sent.
In `@jonesy/src/lsp.rs`:
- Around line 1248-1255: When a target is skipped (needs_analysis is false) the
code only adds cached.panic_count and increments skipped_count but does not
preserve the cached diagnostics, causing editor diagnostics to be cleared later
when state.panic_points is replaced; update the skip path so that when
cache.targets.get(target) returns cached you also merge or append cached
diagnostics (the same diagnostics type the analyzer collects and that is later
used to set state.panic_points/state.diagnostics) into the collection used for
final diagnostics aggregation (alongside adding cached.panic_count and
total_points) so unchanged targets retain their diagnostics; apply the same fix
in the other similar block around lines 1329-1353.
- Around line 911-915: The code currently starts both watcher backends causing
duplicate work; change the startup so only one backend is active: prefer the
native watcher (start_native_file_watcher()) and only call
register_file_watchers() as a fallback when the native watcher is
disabled/unavailable or when the client advertises support for
workspace/didChangeWatchedFiles is missing. Update the logic around
register_file_watchers() and start_native_file_watcher() (use existing functions
register_file_watchers() and start_native_file_watcher(), and any client
capability flag or native-watcher-init result) so register_file_watchers() is
invoked only if the native watcher did not start or the client cannot handle
didChangeWatchedFiles.
- Around line 1172-1247: The skip decision only uses force_full_analysis and
cache.target_needs_analysis(target), so workspace/config invalidation
(workspace_changes) isn't considered; update the needs_analysis computation to
also check whether workspace_changes invalidates this specific target—e.g. add
or call a method like WorkspaceChanges::affects_target(&self, target: &Target)
(or WorkspaceChanges::invalidates_target) and change the condition to: let
needs_analysis = force_full_analysis || cache.target_needs_analysis(target) ||
workspace_changes.affects_target(target); also ensure config-cache/state changes
are surfaced by WorkspaceChanges (add detection logic if missing) so target
reanalysis is triggered when jonesy.toml or Cargo.toml target-path/config
changes.
- Around line 317-364: start_native_file_watcher currently drops watcher_handle
when it goes out of scope, so the underlying RecommendedWatcher is stopped; fix
by adding a field on JonesyLspServer (e.g., watcher_handle:
Option<WatcherHandle>) and store the created watcher_handle into self before
returning so the handle lives for the server lifetime. Update
start_native_file_watcher to assign the produced WatcherHandle to
self.watcher_handle (and ensure debounced_events is called in a way that doesn’t
consume the only ownership of events—either call debounced_events using a
reference/clone of the events or adjust debounced_events to borrow the receiver)
so that WatcherHandle (and its _watcher) is not dropped; refer to symbols
WatcherHandle, start_native_file_watcher, debounced_events, and JonesyLspServer
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6881aac-5b3b-40ea-b69e-59fcd75bb9ec
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
docs/lsp.mdjonesy/Cargo.tomljonesy/src/analysis_cache.rsjonesy/src/file_watcher.rsjonesy/src/lib.rsjonesy/src/lsp.rs
There was a problem hiding this comment.
♻️ Duplicate comments (7)
jonesy/src/file_watcher.rs (3)
127-142:⚠️ Potential issue | 🟠 MajorRegister watches even before the paths exist.
Skipping
watch()whentarget/debugorjonesy.tomlis absent means the first build / first config creation is missed until the server restarts. Watch an existing parent directory instead and keep the path filtering incategorize_path().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jonesy/src/file_watcher.rs` around lines 127 - 142, The current code only calls watcher.watch when config.target_dir or individual config_file exists, which misses creation events; change the logic in the block that registers watches so that for config.target_dir you watch its parent (or the directory itself if it exists) regardless of existence by calling watcher.watch(&config.target_dir.parent().unwrap_or(&config.target_dir), RecursiveMode::NonRecursive) (or similar) and for each config_file always watch its parent directory (even if the file doesn't exist) using watcher.watch(parent, RecursiveMode::NonRecursive); keep path filtering in categorize_path() to ignore unrelated events and handle watcher.watch errors appropriately (log or ignore for optional config_files) while ensuring you reference watcher.watch, config.target_dir, config.config_files, and categorize_path() when making the change.
112-121:⚠️ Potential issue | 🟠 MajorUse a non-blocking send in the notify callback.
blocking_sendcan stall the watcher thread under bursty rebuilds, despite the comment saying overflow should be dropped.try_sendmatches the intended behavior here.Suggested change
let watch_event = categorize_path(&path, &target_dir, &config_files); if let Some(evt) = watch_event { - // Non-blocking send - drop events if channel is full - let _ = tx_clone.blocking_send(evt); + // Non-blocking send - drop events if channel is full + let _ = tx_clone.try_send(evt); } } } Err(e) => { - let _ = tx_clone.blocking_send(WatchEvent::Error(e.to_string())); + let _ = tx_clone.try_send(WatchEvent::Error(e.to_string())); } } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jonesy/src/file_watcher.rs` around lines 112 - 121, The notify callback is using tx_clone.blocking_send (e.g., when sending categorized events from event.paths and WatchEvent::Error) which can stall the watcher thread; change these to non-blocking sends using tx_clone.try_send and silently drop the event on a Full/Err(…) result to preserve the intended "drop on overflow" behavior (ensure both the per-path send and the error-send use try_send and handle the Err case by ignoring/logging without blocking).
171-188:⚠️ Potential issue | 🟠 MajorInclude Windows
.exeartifacts here.On Windows, the main debug output is
target/debug/<name>.exe; this matcher drops it, so the native watcher never emitsBinaryChangedfor executable rebuilds.Suggested change
- Some("rlib") | Some("dylib") | Some("so") | Some("dll") | Some("a") => true, + Some("rlib") | Some("dylib") | Some("so") | Some("dll") | Some("exe") | Some("a") => true,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jonesy/src/file_watcher.rs` around lines 171 - 188, The is_binary_or_library function currently ignores Windows executables and thus won't detect target/debug/<name>.exe; update the matcher in is_binary_or_library to treat "exe" as a binary extension (i.e., include Some("exe") among the true cases) so the watcher emits BinaryChanged for rebuilt Windows executables; locate the match in is_binary_or_library and add "exe" to the library/binary extension list while preserving existing dSYM and None handling.jonesy/src/analysis_cache.rs (1)
23-24:⚠️ Potential issue | 🟠 MajorKeep cache mtimes at subsecond precision.
as_secs()collapses rebuilds that land in the same second, sotarget_needs_analysis()can treat a freshly rebuilt artifact as unchanged and skip re-analysis. Store milliseconds or nanoseconds here instead, and bump the cache schema version when you change the serialized field.Suggested change
pub struct TargetState { /// Path to the target file. pub path: PathBuf, - /// Last modification time (as seconds since epoch). - pub mtime: u64, + /// Last modification time (as milliseconds since epoch). + pub mtime: u128, /// Number of panic points found in last analysis. pub panic_count: usize, } @@ -fn get_mtime(path: &Path) -> Option<u64> { +fn get_mtime(path: &Path) -> Option<u128> { fs::metadata(path) .and_then(|m| m.modified()) .ok() .and_then(|t| t.duration_since(SystemTime::UNIX_EPOCH).ok()) - .map(|d| d.as_secs()) + .map(|d| d.as_millis()) }Also applies to: 244-250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jonesy/src/analysis_cache.rs` around lines 23 - 24, The mtime field currently stores seconds (pub mtime: u64) which collapses subsecond rebuilds; change the serialized mtime to subsecond precision (e.g., milliseconds or nanoseconds) and update all places that write/read it (the struct field in analysis_cache and any serializer/deserializer) to use the chosen unit, adjust conversions where targets are checked (e.g., target_needs_analysis()) to compare using the new unit, and bump the cache schema version used for serialization so old caches are invalidated when the field format changes. Ensure you update any other occurrences of mtime handling (the other instance noted around lines 244-250) to the same unit and add tests verifying subsecond differences are detected.jonesy/src/lsp.rs (3)
317-339:⚠️ Potential issue | 🔴 CriticalKeep the native watcher owned by the server.
watcher_handle.eventsis moved intodebounced_events(), thenwatcher_handlegoes out of scope and drops_watcheras soon as this method returns. That stops native file watching immediately, so the incremental analysis path never fires.Also applies to: 347-359
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jonesy/src/lsp.rs` around lines 317 - 339, The watcher_handle returned by file_watcher::start_watching is being dropped because watcher_handle.events is moved into file_watcher::debounced_events while watcher_handle goes out of scope; keep the native watcher owned by the server to prevent it from being dropped. Fix by storing the handle on the server (e.g., add a field like self.watcher_handle: Option<WatcherHandle> and assign Some(watcher_handle) before calling file_watcher::debounced_events) or otherwise ensure the WatcherHandle lives as long as the server, then pass watcher_handle.events (or a cloned event stream if supported) into debounced_events; apply the same change for the second occurrence around the other debounced_events call (lines 347-359).
1235-1246:⚠️ Potential issue | 🔴 CriticalSkipped targets lose their diagnostics.
The skip path only carries forward
panic_count; it never repopulatespoints_by_filefor unchanged targets. Whenstate.panic_pointsis replaced later, diagnostics from every skipped target disappear from the editor after the first incremental run. Preserve per-target code points/diagnostics for skipped targets before swapping the state.Also applies to: 1321-1329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jonesy/src/lsp.rs` around lines 1235 - 1246, The skip path only accumulates panic_count and drops per-target diagnostics, so when a target is skipped (check via cache.target_needs_analysis) you must also reuse its cached per-file diagnostics: fetch the cached entry from cache.targets (the same place you read panic_count) and merge its points_by_file (or per-target diagnostic map) into the overall points_by_file accumulator before continue; ensure this happens for the loop handling targets (and the same fix for the similar block around lines 1321-1329) so state.panic_points replacement later retains diagnostics from skipped targets.
1166-1239:⚠️ Potential issue | 🟠 MajorConfig-triggered runs still skip unchanged targets.
This path computes
workspace_changes, butneeds_analysisstill only depends on the full-reanalysis flag and binary mtime. Ajonesy.tomlchange or aCargo.tomltarget-path change will enter this function and then skip every unchanged artifact, leaving stale diagnostics until the next rebuild. The new config/workspace invalidation signals need to feed this decision, and the config fingerprints need to be updated after a successful run.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jonesy/src/lsp.rs` around lines 1166 - 1239, The loop still computes needs_analysis solely from force_full_analysis and cache.target_needs_analysis(target), so config/workspace changes (workspace_changes) don't trigger re-analysis; change the needs_analysis computation to also consider whether workspace_changes affects the specific target (e.g., check workspace_changes against the target using available fields like added/removed/changed_binaries, added/removed/changed_libraries or a helper like workspace_changes.affects_target(target)) and set needs_analysis = force_full_analysis || cache.target_needs_analysis(target) || workspace_changes_affects_target; after a successful analysis run update the cache with the new config/workspace fingerprints (e.g., call the AnalysisCache method that updates fingerprints from current_workspace_state or per-target config and then persist the cache via the existing save/load mechanism so future runs see the updated config fingerprints).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@jonesy/src/analysis_cache.rs`:
- Around line 23-24: The mtime field currently stores seconds (pub mtime: u64)
which collapses subsecond rebuilds; change the serialized mtime to subsecond
precision (e.g., milliseconds or nanoseconds) and update all places that
write/read it (the struct field in analysis_cache and any
serializer/deserializer) to use the chosen unit, adjust conversions where
targets are checked (e.g., target_needs_analysis()) to compare using the new
unit, and bump the cache schema version used for serialization so old caches are
invalidated when the field format changes. Ensure you update any other
occurrences of mtime handling (the other instance noted around lines 244-250) to
the same unit and add tests verifying subsecond differences are detected.
In `@jonesy/src/file_watcher.rs`:
- Around line 127-142: The current code only calls watcher.watch when
config.target_dir or individual config_file exists, which misses creation
events; change the logic in the block that registers watches so that for
config.target_dir you watch its parent (or the directory itself if it exists)
regardless of existence by calling
watcher.watch(&config.target_dir.parent().unwrap_or(&config.target_dir),
RecursiveMode::NonRecursive) (or similar) and for each config_file always watch
its parent directory (even if the file doesn't exist) using
watcher.watch(parent, RecursiveMode::NonRecursive); keep path filtering in
categorize_path() to ignore unrelated events and handle watcher.watch errors
appropriately (log or ignore for optional config_files) while ensuring you
reference watcher.watch, config.target_dir, config.config_files, and
categorize_path() when making the change.
- Around line 112-121: The notify callback is using tx_clone.blocking_send
(e.g., when sending categorized events from event.paths and WatchEvent::Error)
which can stall the watcher thread; change these to non-blocking sends using
tx_clone.try_send and silently drop the event on a Full/Err(…) result to
preserve the intended "drop on overflow" behavior (ensure both the per-path send
and the error-send use try_send and handle the Err case by ignoring/logging
without blocking).
- Around line 171-188: The is_binary_or_library function currently ignores
Windows executables and thus won't detect target/debug/<name>.exe; update the
matcher in is_binary_or_library to treat "exe" as a binary extension (i.e.,
include Some("exe") among the true cases) so the watcher emits BinaryChanged for
rebuilt Windows executables; locate the match in is_binary_or_library and add
"exe" to the library/binary extension list while preserving existing dSYM and
None handling.
In `@jonesy/src/lsp.rs`:
- Around line 317-339: The watcher_handle returned by
file_watcher::start_watching is being dropped because watcher_handle.events is
moved into file_watcher::debounced_events while watcher_handle goes out of
scope; keep the native watcher owned by the server to prevent it from being
dropped. Fix by storing the handle on the server (e.g., add a field like
self.watcher_handle: Option<WatcherHandle> and assign Some(watcher_handle)
before calling file_watcher::debounced_events) or otherwise ensure the
WatcherHandle lives as long as the server, then pass watcher_handle.events (or a
cloned event stream if supported) into debounced_events; apply the same change
for the second occurrence around the other debounced_events call (lines
347-359).
- Around line 1235-1246: The skip path only accumulates panic_count and drops
per-target diagnostics, so when a target is skipped (check via
cache.target_needs_analysis) you must also reuse its cached per-file
diagnostics: fetch the cached entry from cache.targets (the same place you read
panic_count) and merge its points_by_file (or per-target diagnostic map) into
the overall points_by_file accumulator before continue; ensure this happens for
the loop handling targets (and the same fix for the similar block around lines
1321-1329) so state.panic_points replacement later retains diagnostics from
skipped targets.
- Around line 1166-1239: The loop still computes needs_analysis solely from
force_full_analysis and cache.target_needs_analysis(target), so config/workspace
changes (workspace_changes) don't trigger re-analysis; change the needs_analysis
computation to also consider whether workspace_changes affects the specific
target (e.g., check workspace_changes against the target using available fields
like added/removed/changed_binaries, added/removed/changed_libraries or a helper
like workspace_changes.affects_target(target)) and set needs_analysis =
force_full_analysis || cache.target_needs_analysis(target) ||
workspace_changes_affects_target; after a successful analysis run update the
cache with the new config/workspace fingerprints (e.g., call the AnalysisCache
method that updates fingerprints from current_workspace_state or per-target
config and then persist the cache via the existing save/load mechanism so future
runs see the updated config fingerprints).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce4cfef4-9f37-4276-80aa-bc8386c0ddb4
📒 Files selected for processing (7)
jonesy/benches/hot_paths.rsjonesy/src/analysis.rsjonesy/src/analysis_cache.rsjonesy/src/call_tree.rsjonesy/src/file_watcher.rsjonesy/src/inline_allows.rsjonesy/src/lsp.rs
Critical fixes: - Store watcher handle to keep native file watcher alive - Preserve diagnostics for skipped (unchanged) targets by merging Major fixes: - Use try_send instead of blocking_send in notify callback - Use millisecond precision for mtime (was seconds, missed sub-second changes) - Watch parent directories when target/config files don't exist at startup - Recognize Windows .exe as binary outputs - Make native watcher primary, LSP watchers fallback only - Include workspace_changes in skip decision for affected targets Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
jonesy/src/analysis_cache.rs (3)
270-280: Consider using a well-tested hash library instead of hand-rolled FNV-1a.While the FNV-1a implementation is correct for change detection, using
std::hash::DefaultHasheror thefnvcrate would be more idiomatic and avoids maintaining custom hashing code.♻️ Alternative using std::hash
+use std::hash::{Hash, Hasher}; +use std::collections::hash_map::DefaultHasher; + /// Simple hash of file content for change detection. fn hash_file_content(path: &Path) -> Option<u64> { let content = fs::read(path).ok()?; - // Simple FNV-1a hash - let mut hash: u64 = 0xcbf29ce484222325; - for byte in content { - hash ^= byte as u64; - hash = hash.wrapping_mul(0x100000001b3); - } - Some(hash) + let mut hasher = DefaultHasher::new(); + content.hash(&mut hasher); + Some(hasher.finish()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jonesy/src/analysis_cache.rs` around lines 270 - 280, The hand-rolled FNV-1a in hash_file_content should be replaced with a well-tested hasher; update hash_file_content to read the file bytes then compute the hash using either std::hash::DefaultHasher (via std::collections::hash_map::DefaultHasher and std::hash::Hasher) or the fnv crate’s FnvHasher, returning the resulting u64. Ensure you keep the same signature (fn hash_file_content(path: &Path) -> Option<u64>), handle fs::read errors the same way, and use the chosen hasher’s finish() as the returned u64 so callers of hash_file_content remain compatible.
247-258:affects_targetname extraction may miss targets with dashes converted to underscores.Cargo converts dashes in crate names to underscores in artifact filenames. If
WorkspaceStatestores names with dashes (fromCargo.toml) but the target path has underscores, this comparison will fail to match.♻️ Proposed fix to normalize names
pub fn affects_target(&self, target_path: &Path) -> bool { let target_name = target_path .file_stem() .and_then(|n| n.to_str()) .map(|n| n.strip_prefix("lib").unwrap_or(n)) + .map(|n| n.replace('-', "_")) .unwrap_or(""); + + let normalize = |s: &String| s.replace('-', "_"); - self.added_binaries.iter().any(|n| n == target_name) - || self.changed_binaries.iter().any(|n| n == target_name) - || self.added_libraries.iter().any(|n| n == target_name) - || self.changed_libraries.iter().any(|n| n == target_name) + self.added_binaries.iter().any(|n| normalize(n) == target_name) + || self.changed_binaries.iter().any(|n| normalize(n) == target_name) + || self.added_libraries.iter().any(|n| normalize(n) == target_name) + || self.changed_libraries.iter().any(|n| normalize(n) == target_name) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jonesy/src/analysis_cache.rs` around lines 247 - 258, The name extraction in affects_target only strips a "lib" prefix and doesn't account for Cargo's dash->underscore conversion, so targets like "my-crate" (artifact "my_crate") won't match; update affects_target to normalize names by converting dashes to underscores before comparisons: after deriving target_name in affects_target call .replace('-', "_") (or otherwise normalize) and when checking against the stored lists (added_binaries, changed_binaries, added_libraries, changed_libraries) compare using the same normalization (e.g., normalize each stored name with .replace('-', "_") or normalize stored names once when populating WorkspaceState) so comparisons succeed regardless of dash/underscore differences.
356-371: Implicit binary detection may miss binaries whenbinarray is empty but non-standard paths are used.If a manifest has no explicit
[[bin]]entries, the code only checks forsrc/main.rs. However, Cargo also supportssrc/bin/*.rsfor additional binaries. This is a minor gap sincecomplete_from_path_and_workspaceinlsp.rshandles this, but thebuild_workspace_statehere may report incomplete workspace state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jonesy/src/analysis_cache.rs` around lines 356 - 371, The implicit-binary detection currently only adds src/main.rs when manifest.bin.is_empty(), which misses binaries in src/bin; update the build_workspace_state logic so that when manifest.bin.is_empty() you also scan crate_root.join("src/bin") (use std::fs::read_dir) and for each entry add a binary: if the entry is a .rs file use its file_stem as the binary name, if the entry is a directory and contains main.rs use the directory name as the binary name; insert these into state.binaries (same as the existing insert pattern) so src/bin/*.rs and src/bin/*/main.rs are recognized in addition to src/main.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@jonesy/src/analysis_cache.rs`:
- Around line 270-280: The hand-rolled FNV-1a in hash_file_content should be
replaced with a well-tested hasher; update hash_file_content to read the file
bytes then compute the hash using either std::hash::DefaultHasher (via
std::collections::hash_map::DefaultHasher and std::hash::Hasher) or the fnv
crate’s FnvHasher, returning the resulting u64. Ensure you keep the same
signature (fn hash_file_content(path: &Path) -> Option<u64>), handle fs::read
errors the same way, and use the chosen hasher’s finish() as the returned u64 so
callers of hash_file_content remain compatible.
- Around line 247-258: The name extraction in affects_target only strips a "lib"
prefix and doesn't account for Cargo's dash->underscore conversion, so targets
like "my-crate" (artifact "my_crate") won't match; update affects_target to
normalize names by converting dashes to underscores before comparisons: after
deriving target_name in affects_target call .replace('-', "_") (or otherwise
normalize) and when checking against the stored lists (added_binaries,
changed_binaries, added_libraries, changed_libraries) compare using the same
normalization (e.g., normalize each stored name with .replace('-', "_") or
normalize stored names once when populating WorkspaceState) so comparisons
succeed regardless of dash/underscore differences.
- Around line 356-371: The implicit-binary detection currently only adds
src/main.rs when manifest.bin.is_empty(), which misses binaries in src/bin;
update the build_workspace_state logic so that when manifest.bin.is_empty() you
also scan crate_root.join("src/bin") (use std::fs::read_dir) and for each entry
add a binary: if the entry is a .rs file use its file_stem as the binary name,
if the entry is a directory and contains main.rs use the directory name as the
binary name; insert these into state.binaries (same as the existing insert
pattern) so src/bin/*.rs and src/bin/*/main.rs are recognized in addition to
src/main.rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e34c2096-7309-4085-acfe-5852f2d53434
📒 Files selected for processing (3)
jonesy/src/analysis_cache.rsjonesy/src/file_watcher.rsjonesy/src/lsp.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- jonesy/src/file_watcher.rs
- Replace hand-rolled FNV-1a hash with std::hash::DefaultHasher - Normalize dash→underscore in affects_target for Cargo compatibility - Scan src/bin/ for implicit binaries in addition to src/main.rs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
jonesy/src/analysis_cache.rs (1)
495-527: Point the hash stability test athash_file_content().Lines 495-527 reimplement the removed FNV path instead of exercising Lines 288-293. This will keep passing even if
hash_file_content()regresses or the hashing algorithm changes again.Suggested fix
#[test] fn test_hash_stability() { - // Same content should produce same hash - let hash1 = { - let mut hash: u64 = 0xcbf29ce484222325; - for byte in b"test content" { - hash ^= *byte as u64; - hash = hash.wrapping_mul(0x100000001b3); - } - hash - }; - - let hash2 = { - let mut hash: u64 = 0xcbf29ce484222325; - for byte in b"test content" { - hash ^= *byte as u64; - hash = hash.wrapping_mul(0x100000001b3); - } - hash - }; - - assert_eq!(hash1, hash2); - - // Different content should produce different hash - let hash3 = { - let mut hash: u64 = 0xcbf29ce484222325; - for byte in b"different content" { - hash ^= *byte as u64; - hash = hash.wrapping_mul(0x100000001b3); - } - hash - }; - - assert_ne!(hash1, hash3); + let path = std::env::temp_dir().join(format!( + "jonesy-hash-test-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + + std::fs::write(&path, b"test content").unwrap(); + let hash1 = hash_file_content(&path).unwrap(); + let hash2 = hash_file_content(&path).unwrap(); + assert_eq!(hash1, hash2); + + std::fs::write(&path, b"different content").unwrap(); + let hash3 = hash_file_content(&path).unwrap(); + assert_ne!(hash1, hash3); + + let _ = std::fs::remove_file(&path); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jonesy/src/analysis_cache.rs` around lines 495 - 527, The test test_hash_stability currently reimplements the FNV hashing loop instead of exercising the actual implementation; update the test to call the hash_file_content() function (and use its output for hash1, hash2, hash3) so the test verifies the real hashing code rather than a duplicated algorithm; ensure you pass the same byte slices ("test content" and "different content") into hash_file_content() and assert_eq!/assert_ne! on its return values so regressions in hash_file_content() are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jonesy/src/analysis_cache.rs`:
- Around line 250-275: The current affects_target function strips a leading
"lib" from every artifact stem (target_name), causing binary names like
"libtool" to be matched as "tool" and thus miss changed_binaries/added_binaries;
fix by not stripping "lib" when comparing against
added_binaries/changed_binaries and only stripping it when comparing against
added_libraries/changed_libraries. Concretely, inside affects_target (symbols:
target_name, normalize, added_binaries, changed_binaries, added_libraries,
changed_libraries) compute one normalized name for binaries (no strip_prefix)
and a second normalized name for libraries (with strip_prefix("lib")), then use
the binary-normalized value for the .iter().any checks against
added_binaries/changed_binaries and the library-normalized value for the checks
against added_libraries/changed_libraries.
- Around line 307-323: The issue is that glob-expanded workspace members are
stored as only the file name (path.file_name()) while explicit members use the
full relative path, causing collisions like "crates/foo" vs "examples/foo". Fix
by pushing the member's relative path (relative to workspace_root or as the
original glob pattern would reference it) instead of file_name: for the glob
branch compute a relative_path =
path.strip_prefix(&workspace_root).unwrap_or(&path).to_string_lossy().to_string()
(or otherwise derive the same relative string) and call
state.members.push(relative_path) so both glob-expanded and explicit branches
store consistent full relative member IDs used by detect_workspace_changes().
- Around line 379-408: The current logic only scans implicit binaries when
manifest.bin.is_empty(), so modify the block in the analysis that manipulates
state.binaries (the code referencing manifest.bin, crate_root, and
state.binaries) to always scan src/main.rs and src/bin/* regardless of
manifest.bin; when inserting discovered names (from src/main.rs, src/bin/*.rs,
and src/bin/*/main.rs) ensure you do not overwrite explicit [[bin]] entries by
checking state.binaries for an existing key first (e.g., use contains_key or
entry(...).or_insert) so explicit manifest bins take precedence while implicit
bins are still picked up.
---
Nitpick comments:
In `@jonesy/src/analysis_cache.rs`:
- Around line 495-527: The test test_hash_stability currently reimplements the
FNV hashing loop instead of exercising the actual implementation; update the
test to call the hash_file_content() function (and use its output for hash1,
hash2, hash3) so the test verifies the real hashing code rather than a
duplicated algorithm; ensure you pass the same byte slices ("test content" and
"different content") into hash_file_content() and assert_eq!/assert_ne! on its
return values so regressions in hash_file_content() are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a6029d6-4022-4800-ac06-5e8f81130f35
📒 Files selected for processing (1)
jonesy/src/analysis_cache.rs
In Rust 2024 edition, Cargo auto-discovers src/main.rs and src/bin/* alongside explicit [[bin]] entries - they merge rather than replace. Use entry().or_insert() to avoid overwriting explicit definitions. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Don't strip 'lib' prefix for binaries (a binary named 'libtool' stays 'libtool') - Use relative paths for glob-expanded workspace members to avoid collisions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This was referenced Mar 29, 2026
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.
Summary
notifycrate for reliable binary change detectiontarget/jonesy/cache.jsonto skip unchanged targetsProblem
LSP file watchers registered via
workspace/didChangeWatchedFilesaren't reliably supported by many IDEs (VS Code, RustRover) for paths outside source directories liketarget/debug/.Solution
Native File Watching (
file_watcher.rs)target/debug/for binaries, rlibs, dylibs, static libsIncremental Analysis Cache (
analysis_cache.rs)Test plan
Closes #166
🤖 Generated with Claude Code