Enable whitaker linting#117
Open
leynos wants to merge 5 commits into
Open
Conversation
Finish the Whitaker rollout by fixing the code paths it flagged across `chutoro-core`, `chutoro-cli`, `chutoro-benches`, provider crates, and `chutoro-test-support`. Replace ambient filesystem access with capability-scoped helpers, remove production `expect` calls from non-test code, add the required module-level inner docs, and split oversized modules so the workspace passes both Clippy and Whitaker.
Contributor
OverviewThis PR enables Whitaker linting repository-wide and applies broad hardening and refactoring to support the linting rules while preserving existing behaviour. Changes span CI/Makefile, capability-based filesystem access, structured error handling (poisoned-lock recovery and fewer unwraps/panics), HNSW CPU build/insert extraction and test reorganisation, MST Kani proofs extraction, CLI and provider API adjustments for capability-scoped I/O, benchmark/test-support utilities (including GitHub Actions output helpers), and developer documentation for Whitaker and quality gates. Whitaker linting & docs
Capability-based filesystem access
Panic-to-error & lock-poisoning hardening
HNSW CPU build & insert refactor
Insert/Commit eviction & healing tests reorganisation
MST & Kani proofs extraction
CLI & provider API changes
Test infrastructure & quality-of-life
Behavioural & API impacts (callouts)
Documentation & developer guidance
Rationale on remaining test similarity (CodeScene)
WalkthroughDescribe repository-wide migration to capability-scoped filesystem APIs, HNSW build refactor into a dedicated module, panics converted to fallible error paths, extensive test reorganisation, and CI/Makefile additions to install and run Whitaker linting. Changes
Sequence Diagram(s)Create a high-level sequence diagram visualising parallel HNSW build with candidate-edge harvest. sequenceDiagram
autonumber
participant Caller as Client
participant CpuHnsw as CpuHnsw::build_with_edges
participant Rayon as Rayon (parallel inserts)
participant Graph as Graph
participant EdgeHarvest as EdgeHarvest::from_parallel_inserts
Caller->>CpuHnsw: call build_with_edges(source, params)
CpuHnsw->>CpuHnsw: build_initial(source, params)
CpuHnsw->>Rayon: spawn parallel inserts (1..items)
Rayon->>CpuHnsw: for each node -> insertion plan & distance cache
CpuHnsw->>Graph: plan insert (read)
Graph-->>CpuHnsw: planner results
CpuHnsw->>EdgeHarvest: collect candidate edges per-node
EdgeHarvest-->>CpuHnsw: reduce into EdgeHarvest
CpuHnsw->>Graph: apply insertions (write)
Graph-->>CpuHnsw: commit results
CpuHnsw-->>Caller: return (CpuHnsw, EdgeHarvest)
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Reviewer's GuideRefactors CpuHnsw build/insertion into a dedicated module, introduces safer locking and error propagation across HNSW/distance cache/tests, adopts capability-based filesystem helpers and GitHub Actions output utilities, and wires Whitaker linting (plus docs/Makefile/CI) into the project. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. chutoro-core/src/hnsw/insert/commit/tests/eviction.rs Comment on file fn eviction_respects_furthest_first_ordering() -> Result<(), HnswError> {
let params = HnswParams::new(2, 4)?;
let max_connections = params.max_connections();
let mut graph = Graph::with_capacity(params, 5);
insert_node(&mut graph, 0, 1, 0)?;
insert_node(&mut graph, 1, 1, 1)?;
insert_node(&mut graph, 2, 1, 2)?;
insert_node(&mut graph, 3, 1, 3)?;
insert_node(&mut graph, 4, 1, 4)?;
let node1 = graph.node_mut(1).expect("node 1 should exist");
node1.neighbours_mut(1).push(2);
node1.neighbours_mut(1).push(3);
add_edge_if_missing(&mut graph, 2, 1, 1);
add_edge_if_missing(&mut graph, 3, 1, 1);
let update = build_update(0, 1, vec![1], max_connections);
let new_node = NewNodeContext { id: 4, level: 1 };
let mut applicator = CommitApplicator::new(&mut graph);
let (reciprocated, _) =
applicator.apply_neighbour_updates(vec![update], max_connections, new_node)?;
applicator.apply_new_node_neighbours(new_node.id, new_node.level, reciprocated)?;
assert_no_edge(&graph, 1, 2, 1);
assert_no_edge(&graph, 2, 1, 1);
assert_has_edge(&graph, 1, 3, 1);
assert_bidirectional_edge(&graph, 0, 1, 1);
Ok(())
}❌ New issue: Code Duplication |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Extract shared helpers for sequential node insertion and commit application in the eviction tests. This removes the repeated setup blocks while keeping the test assertions and the intentional ordering behaviour unchanged.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. chutoro-core/src/hnsw/insert/commit/tests/eviction.rs Comment on file fn benign_deferred_scrub_is_noop_when_edge_already_removed(
params_one_connection: HnswParams,
) -> Result<(), HnswError> {
let max_connections = params_one_connection.max_connections();
let mut graph = Graph::with_capacity(params_one_connection, 5);
insert_sequential_nodes(&mut graph, 5, 1)?;
add_edge_if_missing(&mut graph, 1, 2, 1);
add_edge_if_missing(&mut graph, 2, 1, 1);
let update1 = build_update(0, 1, vec![1], max_connections);
let update2 = build_update(2, 1, vec![3], max_connections);
let new_node = NewNodeContext { id: 4, level: 1 };
apply_and_commit(
&mut graph,
vec![update1, update2],
max_connections,
new_node,
)?;
assert_bidirectional_edge(&graph, 0, 1, 1);
assert_no_edge(&graph, 2, 1, 1);
assert_no_edge(&graph, 1, 2, 1);
assert_bidirectional_edge(&graph, 2, 3, 1);
Ok(())
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
Extract small test helpers for repeated sequential graph construction and bidirectional edge wiring in `eviction.rs`. Reuse those helpers in the duplicated eviction tests while preserving the existing assertions and the ordering-sensitive neighbour setup.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationchutoro-core/src/hnsw/insert/commit/tests/eviction.rs: What lead to degradation?The module contains 2 functions with similar structure: benign_deferred_scrub_is_noop_when_edge_already_removed,multiple_evictions_in_batch_update Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6728
Any further extraction would require a parameterised "scenario runner" that accepts a node count, a Use the paragraph below for CodeScene's suppression input: These two test functions ( Eliminating the remaining overlap would require a parameterised scenario-runner accepting a node count, a variable-length list of bidirectional seed edges, a |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new
Defaultimpl forHnswParamshardcodes field values instead of delegating toHnswParams::new, which risks diverging from the constructor’s validation logic over time; consider havingDefault::defaultcallnew(and handle the infallible case) so changes to validation or parameter relationships remain centralised.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `Default` impl for `HnswParams` hardcodes field values instead of delegating to `HnswParams::new`, which risks diverging from the constructor’s validation logic over time; consider having `Default::default` call `new` (and handle the infallible case) so changes to validation or parameter relationships remain centralised.
## Individual Comments
### Comment 1
<location path="chutoro-test-support/src/github_output.rs" line_range="1-10" />
<code_context>
+//! Capability-based helpers for writing GitHub Actions command files.
+//!
+//! GitHub Actions exposes paths such as `GITHUB_OUTPUT` through environment
+//! variables. These helpers keep the ambient authority boundary narrow by
+//! opening only the parent directory and then appending to the named file
+//! relative to that directory.
+
+use std::env;
+use std::ffi::OsStr;
+use std::io::{self, Write};
+use std::path::Path;
+
+use cap_std::{
+ ambient_authority,
+ fs::{Dir, OpenOptions},
+};
+
+const GITHUB_OUTPUT_DELIMITER: &str = "CHUTORO_EOF";
+
+/// A single key/value pair to append to a GitHub Actions command file.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct OutputPair<'a> {
+ key: &'a str,
+ value: &'a str,
</code_context>
<issue_to_address>
**suggestion (testing):** Add unit tests for github_output helpers, including multiline values and delimiter errors
The new `GITHUB_OUTPUT` helpers have non-trivial behavior (capability-based path handling, no-op when unset/empty, multiline encoding with a custom delimiter, and a specific error path on `CHUTORO_EOF`), but are only indirectly exercised by benchmarks and Kani gates.
Please add a local `#[cfg(test)]` module that at minimum covers:
- `append_if_configured` with `GITHUB_OUTPUT` unset and set to an empty string (both should no-op).
- Writing a simple `key=value` to a temp file and asserting the exact contents.
- Writing a multiline value and asserting the `<<CHUTORO_EOF`/terminator format.
- The error case where a value contains `CHUTORO_EOF` (returning `io::ErrorKind::InvalidInput`).
You can set `GITHUB_OUTPUT` to a temp file and use `cap_std::Dir::open_ambient_dir` in tests to read back the results.
Suggested implementation:
```rust
const GITHUB_OUTPUT_DELIMITER: &str = "CHUTORO_EOF";
/// A single key/value pair to append to a GitHub Actions command file.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct OutputPair<'a> {
key: &'a str,
value: &'a str,
}
#[cfg(test)]
mod tests {
use super::*;
use cap_std::fs::Dir;
use std::env;
use std::io::{self, ErrorKind, Read};
use tempfile::tempdir;
// Helper: read the contents of the GITHUB_OUTPUT file using cap-std.
fn read_github_output(path: &Path) -> io::Result<String> {
let dir_path = path
.parent()
.expect("temp file should have a parent directory");
let file_name = path
.file_name()
.expect("temp file path should have a file name");
let dir = Dir::open_ambient_dir(dir_path, ambient_authority())?;
let mut file = dir.open(file_name)?;
let mut buf = String::new();
file.read_to_string(&mut buf)?;
Ok(buf)
}
#[test]
fn append_if_configured_unset_noop() {
// Ensure GITHUB_OUTPUT is not set.
env::remove_var("GITHUB_OUTPUT");
// Should succeed and not attempt to write anywhere.
// This primarily checks that we don't panic or error.
append_if_configured([OutputPair::new("key", "value")]).unwrap();
}
#[test]
fn append_if_configured_empty_noop() {
// Empty string should be treated the same as unset.
env::set_var("GITHUB_OUTPUT", "");
append_if_configured([OutputPair::new("key", "value")]).unwrap();
}
#[test]
fn writes_simple_key_value() {
let dir = tempdir().unwrap();
let output_path = dir.path().join("github_output.txt");
env::set_var("GITHUB_OUTPUT", &output_path);
append_if_configured([OutputPair::new("mode", "baseline_compare")]).unwrap();
let contents = read_github_output(&output_path).unwrap();
// The simple key=value form should not include a delimiter.
assert_eq!(contents, "mode=baseline_compare\n");
}
#[test]
fn writes_multiline_value_with_delimiter() {
let dir = tempdir().unwrap();
let output_path = dir.path().join("github_output.txt");
env::set_var("GITHUB_OUTPUT", &output_path);
let multiline = "line1\nline2\nline3";
append_if_configured([OutputPair::new("summary", multiline)]).unwrap();
let contents = read_github_output(&output_path).unwrap();
let expected = format!(
"summary<<{delim}\n{val}\n{delim}\n",
delim = GITHUB_OUTPUT_DELIMITER,
val = multiline
);
assert_eq!(contents, expected);
}
#[test]
fn rejects_values_containing_delimiter() {
let dir = tempdir().unwrap();
let output_path = dir.path().join("github_output.txt");
env::set_var("GITHUB_OUTPUT", &output_path);
let bad_value = format!("this contains {delim}", delim = GITHUB_OUTPUT_DELIMITER);
let result = append_if_configured([OutputPair::new("bad", &bad_value)]);
let err = result.expect_err("expected InvalidInput when value contains delimiter");
assert_eq!(err.kind(), ErrorKind::InvalidInput);
}
}
```
These tests assume that:
1. There is a public `append_if_configured` function in this module with a signature compatible with `append_if_configured([OutputPair::new(..)])` (e.g. something like `pub fn append_if_configured<'a, I>(pairs: I) -> io::Result<()> where I: IntoIterator<Item = OutputPair<'a>>`).
2. The implementation treats an unset or empty `GITHUB_OUTPUT` environment variable as a no-op and encodes multiline values using the `key<<CHUTORO_EOF` / `CHUTORO_EOF` format, returning `io::ErrorKind::InvalidInput` when the value contains `CHUTORO_EOF`.
If the actual `append_if_configured` signature differs, adjust the test calls accordingly (e.g. wrap the array in `&[...]` or pass a slice/vector as required).
You will also need a `dev-dependency` on `tempfile` in `Cargo.toml` for `chutoro-test-support` if it is not already present:
```toml
[dev-dependencies]
tempfile = "3"
```
</issue_to_address>
### Comment 2
<location path="chutoro-core/src/hnsw/cpu/build.rs" line_range="29" />
<code_context>
+///
+/// Enables separation of edge harvesting from insertion logic, allowing
+/// non-harvesting paths to avoid allocation overhead.
+trait EdgeCollector {
+ /// Collects edges discovered during a single node insertion.
+ fn collect(&mut self, edges: Vec<CandidateEdge>);
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the EdgeCollector trait and its wrapper types with a single insertion function that takes an optional edge sink to simplify the insertion flow and reduce indirection.
You can drop the `EdgeCollector` abstraction and keep the same behavior with a single optional sink parameter. That removes the trait and the two wrapper structs, and reduces the call depth without changing semantics.
For example, replace `EdgeCollector` / `NoopCollector` / `VecCollector` and `insert_with_collector` with something like:
```rust
fn insert_internal<D: DataSource + Sync>(
&self,
node: usize,
source: &D,
mut edge_sink: Option<&mut Vec<CandidateEdge>>,
) -> Result<(), HnswError> {
let _insertion_guard = self
.insert_mutex
.lock()
.map_err(|_| HnswError::LockPoisoned { resource: "insert mutex" })?;
let level = self.sample_level()?;
let sequence = self.allocate_sequence();
let node_ctx = NodeContext { node, level, sequence };
if self.try_insert_initial(node_ctx, source)? {
self.len.store(1, Ordering::Relaxed);
return Ok(());
}
let cache = &self.distance_cache;
let plan = self.read_graph(|graph| {
graph.insertion_planner().plan(PlanningInputs {
ctx: node_ctx,
params: &self.params,
source,
cache: Some(cache),
})
})?;
if let Some(edges) = edge_sink.as_deref_mut() {
let new_edges = extract_candidate_edges(node, sequence, &plan);
edges.extend(new_edges);
}
let (prepared, trim_jobs) = self.write_graph(|graph| {
let mut executor = graph.insertion_executor();
executor.apply(
node_ctx,
ApplyContext {
params: &self.params,
plan,
},
)
})?;
let trim_results = self.score_trim_jobs(trim_jobs, source)?;
self.write_graph(|graph| {
let mut executor = graph.insertion_executor();
executor.commit(prepared, trim_results)
})?;
self.len.fetch_add(1, Ordering::Relaxed);
Ok(())
}
```
Then the public APIs become thin, direct wrappers:
```rust
pub fn insert<D: DataSource + Sync>(&self, node: usize, source: &D) -> Result<(), HnswError> {
self.insert_internal(node, source, None)
}
fn insert_with_edges<D: DataSource + Sync>(
&self,
node: usize,
source: &D,
) -> Result<Vec<CandidateEdge>, HnswError> {
let mut edges = Vec::new();
self.insert_internal(node, source, Some(&mut edges))?;
Ok(edges)
}
```
`EdgeHarvest::from_parallel_inserts` reads naturally on top of this:
```rust
pub(super) fn from_parallel_inserts<D: DataSource + Sync>(
index: &CpuHnsw,
source: &D,
items: usize,
) -> Result<Self, HnswError> {
let edges = (1..items)
.into_par_iter()
.map(|node| index.insert_with_edges(node, source))
.try_reduce(Vec::new, |mut acc, node_edges| {
acc.extend(node_edges);
Ok(acc)
})?;
Ok(Self::from_unsorted(edges))
}
```
This preserves:
- Zero-cost path when edges aren’t needed (`None` → no allocation/branch inside hot path beyond a single `if let`).
- Existing parallel `map` → `try_reduce` structure for MST harvesting.
But it removes the trait, the two concrete collector types, and the extra layer (`insert_with_collector`), making it easier to follow the insertion flow (`build`/`build_with_edges` → `insert_internal`).
</issue_to_address>
### Comment 3
<location path="chutoro-benches/src/fs_support.rs" line_range="54" />
<code_context>
+ return Err(invalid_path(path));
+ };
+
+ match Dir::open_ambient_dir(parent, ambient_authority()) {
+ Ok(dir) => dir.try_exists(file_name),
+ Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(false),
</code_context>
<issue_to_address>
**issue (review_instructions):** This `match` has three branches; per the guidelines, this conditional should be extracted into a dedicated predicate/helper function.
The `match Dir::open_ambient_dir(parent, ambient_authority())` has three arms (`Ok`, `Err(..) if ..`, and a fallback `Err(..)`), which exceeds the “>2 branches” threshold from the review instructions. To comply, please extract this branching logic into a small helper or predicate function so that the main flow here does not directly contain a 3-way conditional.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
* Move conditionals with >2 branches into a predicate function.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1
to
+10
| //! Capability-based helpers for writing GitHub Actions command files. | ||
| //! | ||
| //! GitHub Actions exposes paths such as `GITHUB_OUTPUT` through environment | ||
| //! variables. These helpers keep the ambient authority boundary narrow by | ||
| //! opening only the parent directory and then appending to the named file | ||
| //! relative to that directory. | ||
|
|
||
| use std::env; | ||
| use std::ffi::OsStr; | ||
| use std::io::{self, Write}; |
There was a problem hiding this comment.
suggestion (testing): Add unit tests for github_output helpers, including multiline values and delimiter errors
The new GITHUB_OUTPUT helpers have non-trivial behavior (capability-based path handling, no-op when unset/empty, multiline encoding with a custom delimiter, and a specific error path on CHUTORO_EOF), but are only indirectly exercised by benchmarks and Kani gates.
Please add a local #[cfg(test)] module that at minimum covers:
append_if_configuredwithGITHUB_OUTPUTunset and set to an empty string (both should no-op).- Writing a simple
key=valueto a temp file and asserting the exact contents. - Writing a multiline value and asserting the
<<CHUTORO_EOF/terminator format. - The error case where a value contains
CHUTORO_EOF(returningio::ErrorKind::InvalidInput).
You can set GITHUB_OUTPUT to a temp file and use cap_std::Dir::open_ambient_dir in tests to read back the results.
Suggested implementation:
const GITHUB_OUTPUT_DELIMITER: &str = "CHUTORO_EOF";
/// A single key/value pair to append to a GitHub Actions command file.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct OutputPair<'a> {
key: &'a str,
value: &'a str,
}
#[cfg(test)]
mod tests {
use super::*;
use cap_std::fs::Dir;
use std::env;
use std::io::{self, ErrorKind, Read};
use tempfile::tempdir;
// Helper: read the contents of the GITHUB_OUTPUT file using cap-std.
fn read_github_output(path: &Path) -> io::Result<String> {
let dir_path = path
.parent()
.expect("temp file should have a parent directory");
let file_name = path
.file_name()
.expect("temp file path should have a file name");
let dir = Dir::open_ambient_dir(dir_path, ambient_authority())?;
let mut file = dir.open(file_name)?;
let mut buf = String::new();
file.read_to_string(&mut buf)?;
Ok(buf)
}
#[test]
fn append_if_configured_unset_noop() {
// Ensure GITHUB_OUTPUT is not set.
env::remove_var("GITHUB_OUTPUT");
// Should succeed and not attempt to write anywhere.
// This primarily checks that we don't panic or error.
append_if_configured([OutputPair::new("key", "value")]).unwrap();
}
#[test]
fn append_if_configured_empty_noop() {
// Empty string should be treated the same as unset.
env::set_var("GITHUB_OUTPUT", "");
append_if_configured([OutputPair::new("key", "value")]).unwrap();
}
#[test]
fn writes_simple_key_value() {
let dir = tempdir().unwrap();
let output_path = dir.path().join("github_output.txt");
env::set_var("GITHUB_OUTPUT", &output_path);
append_if_configured([OutputPair::new("mode", "baseline_compare")]).unwrap();
let contents = read_github_output(&output_path).unwrap();
// The simple key=value form should not include a delimiter.
assert_eq!(contents, "mode=baseline_compare\n");
}
#[test]
fn writes_multiline_value_with_delimiter() {
let dir = tempdir().unwrap();
let output_path = dir.path().join("github_output.txt");
env::set_var("GITHUB_OUTPUT", &output_path);
let multiline = "line1\nline2\nline3";
append_if_configured([OutputPair::new("summary", multiline)]).unwrap();
let contents = read_github_output(&output_path).unwrap();
let expected = format!(
"summary<<{delim}\n{val}\n{delim}\n",
delim = GITHUB_OUTPUT_DELIMITER,
val = multiline
);
assert_eq!(contents, expected);
}
#[test]
fn rejects_values_containing_delimiter() {
let dir = tempdir().unwrap();
let output_path = dir.path().join("github_output.txt");
env::set_var("GITHUB_OUTPUT", &output_path);
let bad_value = format!("this contains {delim}", delim = GITHUB_OUTPUT_DELIMITER);
let result = append_if_configured([OutputPair::new("bad", &bad_value)]);
let err = result.expect_err("expected InvalidInput when value contains delimiter");
assert_eq!(err.kind(), ErrorKind::InvalidInput);
}
}These tests assume that:
- There is a public
append_if_configuredfunction in this module with a signature compatible withappend_if_configured([OutputPair::new(..)])(e.g. something likepub fn append_if_configured<'a, I>(pairs: I) -> io::Result<()> where I: IntoIterator<Item = OutputPair<'a>>). - The implementation treats an unset or empty
GITHUB_OUTPUTenvironment variable as a no-op and encodes multiline values using thekey<<CHUTORO_EOF/CHUTORO_EOFformat, returningio::ErrorKind::InvalidInputwhen the value containsCHUTORO_EOF.
If the actual append_if_configured signature differs, adjust the test calls accordingly (e.g. wrap the array in &[...] or pass a slice/vector as required).
You will also need a dev-dependency on tempfile in Cargo.toml for chutoro-test-support if it is not already present:
[dev-dependencies]
tempfile = "3"
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
chutoro-core/src/hnsw/insert/test_helpers.rs (1)
9-18:⚠️ Potential issue | 🟠 MajorReturn
boolto expose silent failures in release builds.The
debug_assert!(false, ...)pattern fires only in debug builds; in release mode the function silently returns without indicating the edge was not added. Callers cannot detect this failure, which allows tests to pass incorrectly when run with--release.Return
boolto signal success, matchingtry_connect_unreachable_nodeelsewhere in the codebase.Proposed fix
-pub(crate) fn add_edge_if_missing(graph: &mut Graph, origin: usize, target: usize, level: usize) { - let Some(node) = graph.node_mut(origin) else { - debug_assert!(false, "node {origin} should exist"); - return; - }; +pub(crate) fn add_edge_if_missing(graph: &mut Graph, origin: usize, target: usize, level: usize) -> bool { + let Some(node) = graph.node_mut(origin) else { + return false; + }; let neighbours = node.neighbours_mut(level); if !neighbours.contains(&target) { neighbours.push(target); } + true }Update all 29 call sites to assert or propagate:
assert!(add_edge_if_missing(&mut graph, origin, target, level), "node {origin} should exist");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@chutoro-core/src/hnsw/insert/test_helpers.rs` around lines 9 - 18, Change add_edge_if_missing to return a bool indicating success instead of silently returning: when origin node is missing return false, otherwise add the target if not present and return true (true also if target already existed). Update all callers (29 sites) to assert or propagate the result (e.g., assert the function returns true with a message that node {origin} should exist) so failures are visible in release builds; keep the function name add_edge_if_missing and match the success semantics used by try_connect_unreachable_node elsewhere in the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@chutoro-benches/src/fs_support.rs`:
- Around line 48-58: The functions try_exists and open_parent_dir currently
reopen caller-supplied paths using Dir::open_ambient_dir(...,
ambient_authority()), which defeats the module's claimed capability-scoped
model; fix by removing ambient_authority usage: either change the APIs to accept
a caller-provided capability Dir (e.g., accept &Dir or cap-std::fs::Dir in
try_exists and open_parent_dir) and use that Dir to query or open the parent, or
if you must keep path-based APIs validate inputs (reject absolute paths and any
path that contains .. components via Path::is_absolute() and
Component::ParentDir checks) before opening and then open only relative safe
paths without ambient_authority; also update the module-level documentation to
state the chosen model (trusted Dir parameter or explicit ambient access) so
callers know the security semantics.
In `@chutoro-cli/src/cli/output.rs`:
- Line 38: Replace the brittle byte-length check on the test buffer with a
semantic content assertion: instead of assert_eq!(buffer.into_inner().len(), 38)
call buffer.into_inner() to get the Vec<u8>, convert to a UTF-8 string (e.g.,
String::from_utf8(...).unwrap()) and assert on the meaningful output (either
assert_eq! with the expected formatted string or assert!(s.contains("...")) for
key substrings). Update the assertion near the test that uses
buffer/into_inner() in chutoro-cli/src/cli/output.rs so it validates actual
output content rather than byte count.
In `@chutoro-cli/src/logging.rs`:
- Around line 61-64: Add a unit test that intentionally poisons the global Mutex
used by INIT_GUARD and then calls init_logging() to ensure the recovery path
works: in the local test module, acquire INIT_GUARD.get_or_init(||
Mutex::new(())) and cause a poison (e.g., spawn a thread that panics while
holding the lock or call lock().unwrap_err().into_inner()), drop it so the mutex
becomes poisoned, then call init_logging() and assert it returns/does not panic;
mirror this for any other places where the same poisoned-guard logic is used
(the other init paths referenced around the other ranges) so the regression is
covered.
In `@chutoro-core/src/hnsw/cpu/build.rs`:
- Around line 251-287: The collector currently always causes
extract_candidate_edges(...) to run even for NoopCollector; add a
needs_edges(&self) -> bool method to the EdgeCollector trait and implement it to
return false for NoopCollector and true for VecCollector, then change
insert_with_collector (around the call site using extract_candidate_edges, node,
sequence, &plan) to call collector.needs_edges() and only call
extract_candidate_edges(...) and collector.collect(...) when true; reference
EdgeCollector::needs_edges, NoopCollector, VecCollector, insert_with_collector,
extract_candidate_edges, and collect when making the changes.
In `@chutoro-core/src/hnsw/cpu/tests.rs`:
- Around line 24-46: Replace the sleep-based synchronization with an explicit
mpsc handshake: create an mpsc::channel and send a start signal from the spawned
thread before calling index.insert (use a sender like started_tx.send(()) in the
thread), then in the main thread wait for that signal with
started_rx.recv_timeout(Duration::from_secs(1)) instead of thread::sleep; keep
the existing started and finished AtomicBools and the handle logic, and ensure
the send/recv errors are unwrapped with expect messages so the test fails
deterministically if the worker doesn't start.
In `@chutoro-core/src/hnsw/invariants/tests/structural.rs`:
- Around line 6-51: The test no_self_loops_after_construction is mutating Graph
internals and then re-reading neighbour vectors instead of exercising the
invariants checker; update the test to construct the Graph via
Graph::with_capacity, insert_first and attach_node as before but do not manually
push into node.neighbours; instead call the invariants module's public check
function (use the project's invariants API — e.g. invariants::check_graph or the
crate's exported checker) to validate the graph and assert it reports self-loop
errors when present (and likewise update duplicate_neighbour_is_detectable and
the other listed tests to drive the invariants checker rather than reading
neighbours via node_mut/nodes_iter). Ensure you reference the same setup steps
(Graph::with_capacity, insert_first, attach_node) so the checker is exercised
with the constructed graph.
In `@chutoro-core/src/hnsw/tests/property/idempotency_property.rs`:
- Around line 154-163: The helper currently panics via .expect("graph snapshot
must acquire the test graph lock"); change the helper to return a
Result<GraphSnapshot, E> (use the TestCaseError or the existing error type used
by the property test) and propagate the failure instead of calling expect: call
index.inspect_graph(...)? (or map_err into TestCaseError) and use ? to propagate
errors; adjust the helper's signature to return Result<GraphSnapshot, _> and
update callers in the property test to propagate the Result so property failures
surface as TestCaseError. Ensure you reference the existing symbols
inspect_graph, GraphSnapshot, snapshot_node, entry, and nodes when making the
changes.
In `@chutoro-core/src/hnsw/tests/property/mutation_property.rs`:
- Around line 157-162: Replace the panic-based test helper with a fallible API:
change fn heal_graph(index: &CpuHnsw) to return Result<(), TestCaseError> and
replace the expect(...) call inside heal_graph with mapping the healing error
into a TestCaseError (e.g., index.heal_for_test().map_err(|e|
TestCaseError::fail(format!("test-only graph healing failed: {e}")))). Then
update the callers that previously invoked heal_graph(...) to use the fallible
form and propagate errors with ? (e.g., heal_graph(ctx.index)?), so the property
test returns the TestCaseError instead of aborting.
In `@chutoro-core/src/mst/kani.rs`:
- Around line 49-57: The kani_find_root function currently performs union-find
root lookup without path compression; update the code by adding a clear doc
comment above kani_find_root stating that path compression is intentionally
omitted for Kani verification (e.g., due to bounded input size and simplicity
for verification) so future maintainers understand this simplification, and
optionally note that a standard path-compressing variant should be used in
production (reference the kani_find_root function name in the comment).
- Around line 95-96: Replace the .expect() call on parallel_kruskal_from_edges
with explicit error handling: match on parallel_kruskal_from_edges(node_count,
edges.iter()), bind Ok(f) to forest and return early on Err(_) (since invalid
inputs are not interesting for the Kani proof); update any subsequent references
to use the bound forest variable. Ensure you modify the harness code that
currently declares let forest = parallel_kruskal_from_edges(...) and remove the
expect usage so the harness is gated by #[cfg(kani)] and handles errors
explicitly.
In `@chutoro-core/src/result.rs`:
- Around line 52-55: Add a regression unit test that ensures
ClusteringResult::from_assignments preserves sparse and duplicate cluster
identifiers and reports cluster_count() as the number of distinct IDs: create a
test (e.g., from_assignments_preserves_sparse_ids_and_counts_distinct_clusters)
that constructs a ClusteringResult via
ClusteringResult::from_assignments(vec![ClusterId::new(7), ClusterId::new(7),
ClusterId::new(42)]), assert that result.assignments() returns the same sequence
of ClusterId values and assert that result.cluster_count() == 2 so this
behaviour cannot regress.
In `@chutoro-providers/dense/src/ingest.rs`:
- Around line 97-109: The code currently calls first_null_value_index(floats,
dimension) unconditionally which scans all dimension slots; check
floats.null_count() > 0 first and only call first_null_value_index when nulls
exist so the hot ingest path avoids an O(rows×dimension) bitmap scan—i.e., move
the null_count() check before invoking first_null_value_index and, if
null_count() > 0, call first_null_value_index and return
DenseMatrixProviderError::NullValue as before, otherwise skip the scan and
continue.
In `@chutoro-providers/dense/src/provider.rs`:
- Around line 63-65: The public API change made to try_from_parquet_path (now
taking a &Dir instead of a bare path) is breaking for consumers; update the
project's changelog and migration notes to document this migration path and
either show how to obtain a capability-scoped directory handle (Dir) from the
previous path-based usage or provide guidance to call the new signature. In the
notes reference the function name try_from_parquet_path and describe the minimal
code change consumers must perform (how to construct or acquire a &Dir or use a
compatibility wrapper), and optionally mention adding a short-lived adapter
function that accepts a Path and converts it to a Dir to ease migration.
In `@chutoro-providers/dense/src/tests/errors.rs`:
- Line 1: Update the module-level doc comment in tests/errors.rs to explicitly
state which error type's From trait implementations are being tested: mention
the From trait and the concrete error type under test (e.g., "tests for
From<...> implementations for <ErrorType>"). Edit the top-line doc comment so it
reads something like "Error conversion coverage: tests for From trait
implementations for <ErrorType>." to make the intent precise and refer to the
specific error type under test.
In `@chutoro-test-support/src/github_output.rs`:
- Around line 87-149: Add unit tests that exercise append_if_configured and the
encoding branches: create tests for (1) GITHUB_OUTPUT unset (ensure function
returns Ok and nothing created), (2) GITHUB_OUTPUT set to empty string (ensure
Ok and no file written), (3) normal single-line key/value (verify "key=value"
appended), (4) multiline value uses heredoc form (verify key<<DELIM, value,
DELIM sequence), (5) value containing GITHUB_OUTPUT_DELIMITER returns an
io::Error, and (6) path splitting with a parent directory (set GITHUB_OUTPUT to
a temp dir + filename and verify file is created inside parent). In each test
set/clear the environment variable, use a temporary directory/file for
GITHUB_OUTPUT, call append_if_configured(entries), then read the target file (or
assert no file) and assert contents or error; reference functions
append_if_configured, write_github_output_value, split_output_path,
open_output_file and constant GITHUB_OUTPUT_DELIMITER to locate the code paths
to cover.
In `@chutoro-test-support/tests/benchmark_regression_gate_cli.rs`:
- Around line 176-182: Change the helper functions (e.g., find_in_deps and the
similar helper at lines 184–189) to return Result<Utf8PathBuf, anyhow::Error>
(or another suitable error type) instead of Option, replace all uses of ok()?
with ? so failures from Dir::open_ambient_dir, dir.entries(), entry metadata,
and is_matching_binary are propagated, and adjust callers (such as binary_path)
to use ? and add contextual .with_context(...) messages like "failed to locate
benchmark_regression_gate binary"; ensure is_matching_binary either returns
Result<Option<Utf8PathBuf>, _> or its errors are mapped into the new Result
chain so no IO/iteration errors are collapsed to None.
In `@Makefile`:
- Line 8: Update the CLIPPY_FLAGS Makefile variable to include --workspace and
deny warnings so `make lint` runs `cargo clippy` over the entire workspace and
fails on any Clippy warnings; specifically modify the CLIPPY_FLAGS definition
(the variable named CLIPPY_FLAGS) to prepend `--workspace` (e.g., `--workspace
--all-targets --all-features --`) and ensure `-D warnings` is included (either
appended to CLIPPY_FLAGS or ensured via RUST_FLAGS) so the lint invocation
matches the project guideline.
- Line 26: The make test target currently invokes `RUSTFLAGS="$(RUST_FLAGS)"
$(CARGO) nextest run --profile $(NEXTEST_PROFILE) --all-targets --all-features
$(BUILD_JOBS)`; change it to run the canonical workspace test command by
replacing that invocation with `RUSTFLAGS="$(RUST_FLAGS)" $(CARGO) test
--workspace` so `make test` executes `cargo test --workspace` as required by the
documented test gate contract and guidelines.
In `@README.md`:
- Around line 97-98: The metrics bullet in README.md has a broken wrap that
leaves an opening parenthesis alone and may exceed 80 columns; edit the metrics
list item containing the parenthetical link text "users' guide § feature flags"
so the entire parenthesis stays attached to the preceding text and reflow the
bullet to 80 columns or less (wrap the sentence so the "(" does not stand alone
and the inline link remains inside the same parenthesis); ensure the final line
reads as a single, readable parenthetical reference and conforms to the
80-column wrap guideline.
---
Outside diff comments:
In `@chutoro-core/src/hnsw/insert/test_helpers.rs`:
- Around line 9-18: Change add_edge_if_missing to return a bool indicating
success instead of silently returning: when origin node is missing return false,
otherwise add the target if not present and return true (true also if target
already existed). Update all callers (29 sites) to assert or propagate the
result (e.g., assert the function returns true with a message that node {origin}
should exist) so failures are visible in release builds; keep the function name
add_edge_if_missing and match the success semantics used by
try_connect_unreachable_node elsewhere in the codebase.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: d6b3ac9a-49f8-4ed9-9bcf-3898410dab80
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (91)
.github/workflows/ci.ymlMakefileREADME.mdchutoro-benches/Cargo.tomlchutoro-benches/benches/hnsw.rschutoro-benches/benches/hnsw_ef_sweep.rschutoro-benches/src/clustering_quality.rschutoro-benches/src/ef_sweep.rschutoro-benches/src/fs_support.rschutoro-benches/src/lib.rschutoro-benches/src/params.rschutoro-benches/src/profiling/memory_sampler.rschutoro-benches/src/profiling/mod.rschutoro-benches/src/recall.rschutoro-benches/src/source/mnist/mod.rschutoro-benches/src/source/mnist/tests.rschutoro-cli/Cargo.tomlchutoro-cli/src/cli/commands.rschutoro-cli/src/cli/mod.rschutoro-cli/src/cli/output.rschutoro-cli/src/cli/test_fixtures.rschutoro-cli/src/cli/test_helpers.rschutoro-cli/src/logging.rschutoro-core/src/chutoro.rschutoro-core/src/cpu_pipeline.rschutoro-core/src/hnsw/cpu/build.rschutoro-core/src/hnsw/cpu/mod.rschutoro-core/src/hnsw/cpu/test_helpers.rschutoro-core/src/hnsw/cpu/tests.rschutoro-core/src/hnsw/distance_cache.rschutoro-core/src/hnsw/graph/test_helpers/mod.rschutoro-core/src/hnsw/helpers.rschutoro-core/src/hnsw/insert/commit/tests.rschutoro-core/src/hnsw/insert/commit/tests/eviction.rschutoro-core/src/hnsw/insert/commit/tests/healing.rschutoro-core/src/hnsw/insert/executor/tests/mod.rschutoro-core/src/hnsw/insert/executor/tests/trimming_fixtures.rschutoro-core/src/hnsw/insert/test_helpers.rschutoro-core/src/hnsw/invariants/degree_bounds.rschutoro-core/src/hnsw/invariants/layer_consistency.rschutoro-core/src/hnsw/invariants/mod.rschutoro-core/src/hnsw/invariants/tests.rschutoro-core/src/hnsw/invariants/tests/structural.rschutoro-core/src/hnsw/node.rschutoro-core/src/hnsw/params.rschutoro-core/src/hnsw/tests/build.rschutoro-core/src/hnsw/tests/cache.rschutoro-core/src/hnsw/tests/property/edge_harvest_output/harvest.rschutoro-core/src/hnsw/tests/property/edge_harvest_output/suite.rschutoro-core/src/hnsw/tests/property/edge_harvest_property.rschutoro-core/src/hnsw/tests/property/edge_harvest_suite/connectivity.rschutoro-core/src/hnsw/tests/property/edge_harvest_suite/degree_ceiling.rschutoro-core/src/hnsw/tests/property/edge_harvest_suite/determinism.rschutoro-core/src/hnsw/tests/property/edge_harvest_suite/rnn_uplift.rschutoro-core/src/hnsw/tests/property/graph_metrics.rschutoro-core/src/hnsw/tests/property/graph_topologies/tests.rschutoro-core/src/hnsw/tests/property/graph_topology_tests/tests.rschutoro-core/src/hnsw/tests/property/idempotency_property.rschutoro-core/src/hnsw/tests/property/mutation_property.rschutoro-core/src/hnsw/tests/property/mutation_property/mutation_pools.rschutoro-core/src/hnsw/tests/property/search_config.rschutoro-core/src/hnsw/tests/property/tests.rschutoro-core/src/hnsw/tests/search.rschutoro-core/src/hnsw/validate.rschutoro-core/src/memory.rschutoro-core/src/mst/kani.rschutoro-core/src/mst/mod.rschutoro-core/src/mst/tests.rschutoro-core/src/mst/tests/forest.rschutoro-core/src/result.rschutoro-core/tests/common/mod.rschutoro-core/tests/functional_ari_nmi.rschutoro-providers/dense/Cargo.tomlchutoro-providers/dense/src/ingest.rschutoro-providers/dense/src/provider.rschutoro-providers/dense/src/source.rschutoro-providers/dense/src/tests/errors.rschutoro-providers/dense/src/tests/provider.rschutoro-providers/text/src/lib.rschutoro-test-support/Cargo.tomlchutoro-test-support/src/bin/benchmark_regression_gate.rschutoro-test-support/src/bin/kani_nightly_gate.rschutoro-test-support/src/ci/benchmark_regression_profile.rschutoro-test-support/src/ci/nightly_gate.rschutoro-test-support/src/ci/property_test_profile.rschutoro-test-support/src/github_output.rschutoro-test-support/src/lib.rschutoro-test-support/tests/benchmark_regression_gate_cli.rschutoro-test-support/tests/kani_nightly_gate_cli.rsdocs/developers-guide.mddocs/whitaker-users-guide.md
Comment on lines
+48
to
+58
| pub(crate) fn try_exists(path: &Path) -> io::Result<bool> { | ||
| let parent = path.parent().unwrap_or_else(|| Path::new(".")); | ||
| let Some(file_name) = path.file_name() else { | ||
| return Err(invalid_path(path)); | ||
| }; | ||
|
|
||
| match Dir::open_ambient_dir(parent, ambient_authority()) { | ||
| Ok(dir) => dir.try_exists(file_name), | ||
| Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(false), | ||
| Err(error) => Err(error), | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
cap-std open_ambient_dir ambient_authority capability boundary documentation
💡 Result:
In cap-std, a capability-based Rust standard library alternative from the Bytecode Alliance, open_ambient_dir (available in both cap-primitives::fs and cap_std::fs::Dir) opens a directory using the host process's full ambient authority, bypassing capability boundaries. Ambient authority refers to the process's implicit permissions to access resources by name (e.g., absolute paths) without explicit capabilities, contrasting with cap-std's model where access requires a Dir capability. The capability boundary is enforced by requiring a Dir for relative path operations within its subtree, preventing traversal outside (e.g., via "..", symlinks, or absolute paths). open_ambient_dir explicitly opts out of this by requiring an AmbientAuthority token, obtained via cap_std::ambient_authority. This design makes ambient authority uses explicit and auditable, originally marked unsafe but replaced with AmbientAuthority for better ergonomics. It protects against path traversal (CWE-22) in capability-aware code. Usage example: use cap_std::fs::Dir; use cap_std::ambient_authority; let dir = Dir::open_ambient_dir("/path/to/dir", ambient_authority)?; This grants a Dir to any accessible path, not sandboxed.
Citations:
- 1: https://docs.rs/cap-primitives/latest/cap_primitives/fs/fn.open_ambient_dir.html
- 2: https://docs.rs/cap-std/latest/cap_std/
- 3: https://github.com/bytecodealliance/cap-std
- 4: https://docs.rs/cap-std/latest/cap_std/fs/struct.Dir.html
- 5: Is
unsafethe best way to indicate ambient authority? bytecodealliance/cap-std#141 - 6: https://docs.rs/cap-primitives
- 7: https://github.com/sunfishcode/ambient-authority
- 8: https://github.com/sunfishcode/ambient-authority/blob/main/README.md
🏁 Script executed:
# Examine the full fs_support.rs file to understand the complete context
cd chutoro-benches/src && wc -l fs_support.rs && cat -n fs_support.rsRepository: leynos/chutoro
Length of output: 3653
🏁 Script executed:
# Check if there are any validation mechanisms for paths before reopening
rg -A 5 -B 5 "open_ambient_dir|ambient_authority" chutoro-benches/src/Repository: leynos/chutoro
Length of output: 4559
Update module documentation and stop using ambient authority to reopen caller-supplied paths.
The module claims "Capability-scoped filesystem helpers" but actually uses Dir::open_ambient_dir(..., ambient_authority()) to reopen paths provided by callers. The ambient_authority() API explicitly opts out of cap-std's capability boundary model—it grants a Dir to any path the host process can access, not sandboxed. This defeats the stated purpose. Either:
- Accept a trusted
Dirfrom the caller instead of accepting raw paths - Validate paths to reject absolute paths and
..segments before opening - Update the module documentation to acknowledge unrestricted ambient access
This applies to try_exists (lines 48–58) and open_parent_dir (lines 84–95).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@chutoro-benches/src/fs_support.rs` around lines 48 - 58, The functions
try_exists and open_parent_dir currently reopen caller-supplied paths using
Dir::open_ambient_dir(..., ambient_authority()), which defeats the module's
claimed capability-scoped model; fix by removing ambient_authority usage: either
change the APIs to accept a caller-provided capability Dir (e.g., accept &Dir or
cap-std::fs::Dir in try_exists and open_parent_dir) and use that Dir to query or
open the parent, or if you must keep path-based APIs validate inputs (reject
absolute paths and any path that contains .. components via Path::is_absolute()
and Component::ParentDir checks) before opening and then open only relative safe
paths without ambient_authority; also update the module-level documentation to
state the chosen model (trusted Dir parameter or explicit ambient access) so
callers know the security semantics.
Comment on lines
+97
to
+98
| ( | ||
| [users' guide § feature flags](docs/users-guide.md#feature-flags-and-execution-strategies)). |
Contributor
There was a problem hiding this comment.
Fix bullet wrapping and inline link formatting in the metrics item.
Keep the parenthetical link readable and wrap the bullet to 80 columns.
The current split leaves ( alone and likely exceeds the wrap limit.
Proposed patch
- Optional `metrics` crate integration for distance-cache telemetry
- (
- [users' guide § feature flags](docs/users-guide.md#feature-flags-and-execution-strategies)).
+ ([users' guide § feature flags]
+ (docs/users-guide.md#feature-flags-and-execution-strategies)).Triage: [type:syntax/md] [type:docstyle]
As per coding guidelines, "Markdown paragraphs and bullet points must be wrapped
at 80 columns."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ( | |
| [users' guide § feature flags](docs/users-guide.md#feature-flags-and-execution-strategies)). | |
| ([users' guide § feature flags] | |
| (docs/users-guide.md#feature-flags-and-execution-strategies)). |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 97 - 98, The metrics bullet in README.md has a broken
wrap that leaves an opening parenthesis alone and may exceed 80 columns; edit
the metrics list item containing the parenthetical link text "users' guide §
feature flags" so the entire parenthesis stays attached to the preceding text
and reflow the bullet to 80 columns or less (wrap the sentence so the "(" does
not stand alone and the inline link remains inside the same parenthesis); ensure
the final line reads as a single, readable parenthetical reference and conforms
to the 80-column wrap guideline.
Apply the verified review fixes across the workspace and keep the branch aligned with the current gate contract. - Route HnswParams::default through the constructor and add regression coverage around logging, result handling, and GitHub output helpers - Simplify HNSW insertion edge harvesting, make property-test helpers fallible, and surface missing edge origins in tests - Tighten filesystem helper semantics, document the dense provider capability migration, and update the Makefile gates
There was a problem hiding this comment.
Gates Failed
(1 file with Bumpy Road Ahead)
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| ingest.rs | 1 critical rule | 10.00 → 9.84 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| github_output.rs | 1 advisory rule | 9.39 | Suppress |
Absence of Expected Change Pattern
- chutoro/chutoro-core/src/hnsw/node.rs is usually changed with: chutoro/chutoro-core/src/hnsw/graph/core.rs
- chutoro/chutoro-core/src/hnsw/tests/property/tests.rs is usually changed with: chutoro/chutoro-core/src/hnsw/tests/property/mod.rs
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Comment on lines
+98
to
+108
| if let Some(value_index) = first_null_value_index(floats, dimension) { | ||
| return Err(DenseMatrixProviderError::NullValue { | ||
| row: absolute_row, | ||
| value_index, | ||
| }); | ||
| } | ||
| return Err(DenseMatrixProviderError::Arrow( | ||
| ArrowError::InvalidArgumentError( | ||
| "Float32Array reported null values but no null index was found".to_owned(), | ||
| ), | ||
| )); |
There was a problem hiding this comment.
❌ New issue: Bumpy Road Ahead
copy_list_values has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is 2 blocks per function
Comment on lines
+225
to
+238
| fn append_if_configured_unset_noop() { | ||
| let temp_dir = tempdir().expect("tempdir must be created"); | ||
| let output_path = temp_dir.path().join("github_output.txt"); | ||
|
|
||
| with_github_output_env(None, || { | ||
| append_if_configured(&[OutputPair::new("mode", "baseline_compare")]) | ||
| }) | ||
| .expect("unset GITHUB_OUTPUT must be a no-op"); | ||
|
|
||
| assert!( | ||
| !output_path.exists(), | ||
| "unset GITHUB_OUTPUT must not create an output file" | ||
| ); | ||
| } |
There was a problem hiding this comment.
❌ New issue: Code Duplication
The module contains 2 functions with similar structure: tests.append_if_configured_empty_noop,tests.append_if_configured_unset_noop
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@chutoro-benches/src/fs_support.rs`:
- Around line 98-164: Add unit and integration tests that exercise
validate_scoped_path, split_scope, open_existing_parent_dir, and open_parent_dir
to lock in expected scoped-path and parent-resolution behavior: write tests that
assert validate_scoped_path rejects paths containing ParentDir or Prefix (e.g.,
".." and Windows prefixes), that split_scope returns correct (Dir, relative
PathBuf) for absolute vs relative inputs and treats root/empty parents properly,
that open_existing_parent_dir returns Ok(Some(dir)) for existing parents,
Ok(None) for missing parents (and that try_exists/metadata behave accordingly),
and that open_parent_dir creates parents only when create_parent=true and
returns the file_name via (Dir, &OsStr); place tests in the module’s test suite
using temporary dirs or cap-std ambient_dir helpers to avoid touching real FS
and include regression cases reproducing the bug so the tests fail before the
fix and pass after.
- Around line 98-115: The code currently rejects any Component::Prefix in
validate_scoped_path, which prevents Windows absolute paths (drive-letter
prefixes) from being accepted by split_scope; either (A) implement Windows-aware
handling: in validate_scoped_path allow Component::Prefix on windows
(cfg(target_family = "windows")) and in split_scope detect and preserve the
prefix (e.g., extract the drive prefix and treat it as the root component when
computing root_path and opening Dir via Dir::open_ambient_dir using the prefixed
root) and ensure Path::is_absolute behavior aligns with the prefix handling, or
(B) narrow the documented contract and cfg-gate the module: update docs to state
absolute-path support is POSIX-only and add cfg attributes so Component::Prefix
is rejected only on non-windows targets (leave validate_scoped_path as-is on
non-windows). Ensure you reference and modify validate_scoped_path and
split_scope accordingly and adjust tests/docs to reflect the chosen approach.
In `@chutoro-core/src/hnsw/cpu/build.rs`:
- Around line 219-266: The insert_mutex is held across planning, scoring and
commit which serializes all build tasks; instead acquire the lock only for the
minimal shared-mutation sections (e.g. around allocate_sequence(), the initial
insertion check try_insert_initial(node_ctx, ...), and the final len updates),
then drop it before calling sample_level(), read_graph(...) planning,
score_trim_jobs(...), and write_graph(...) work; specifically, move the lock so
it only protects the sequence allocation/initial-insert path and the
len.fetch_add/fetch_store updates, allowing read_graph, extract_candidate_edges,
score_trim_jobs, and write_graph executor.apply/commit to run without the
long-lived insert_mutex.
- Around line 176-193: The example shows duplicate insertion because
CpuHnsw::build(...) already inserts all source items; change the example to
demonstrate incremental insertion by either creating an empty index (use
CpuHnsw::new(HnswParams) instead of CpuHnsw::build(...)) and then calling
insert(1, &data), or keep build(...) and call insert with an id that wasn’t
included in the source (e.g., insert(3, &data)); update the doc example text
accordingly so it uses CpuHnsw::new or a non-duplicated id and matches the
actual behavior of CpuHnsw::build and CpuHnsw::insert.
In `@chutoro-core/src/hnsw/cpu/tests.rs`:
- Around line 15-54: Add a regression test that exercises a poisoned
insert_mutex and asserts the insert returns HnswError::LockPoisoned { resource:
"insert mutex" }: create a test similar to insert_waits_for_mutex which spawns a
thread that locks index.insert_mutex and then panics (to poison the lock),
join/ignore the panicking thread, then call index.insert(...) and assert the
returned Err matches HnswError::LockPoisoned with resource "insert mutex" (use
CpuHnsw::insert and the HnswError enum to match the error); ensure the test uses
the same TestSource/HnswParams setup and Atomic flags to coordinate if needed so
the lock is poisoned before calling insert.
- Around line 33-49: The test currently only checks the finished AtomicBool and
can pass if the worker is unscheduled; modify the test so the worker sends a
completion signal (use finished_tx/finshed_rx) after index.insert(...) and while
holding the guard assert that no completion message is received by calling
finished_rx.recv_timeout(...) and expecting a timeout, then drop(guard) and
assert that finished_rx.recv() or recv_timeout now returns the completion;
reference the worker's index.insert call, the started_tx/started_rx handshake,
the finished_tx/finished_rx completion channel, and the mutex guard to locate
where to add the send and the recv_timeout assertions.
In `@chutoro-core/src/hnsw/insert/commit/tests/eviction.rs`:
- Around line 76-83: The test helper insert_sequential_nodes currently calls
u64::try_from(node_id).expect(...), which panics; change this to propagate the
conversion error instead of panicking so the helper remains fallible—replace the
expect with a fallible conversion (e.g. use u64::try_from(node_id)? or
u64::try_from(node_id).map_err(|e| HnswError::from(e)) as appropriate) and
adjust insert_sequential_nodes to return Err on conversion failure; keep
references to insert_node and the existing Result<(), HnswError> signature.
In `@chutoro-core/src/hnsw/invariants/tests/structural.rs`:
- Around line 11-29: Extract the repeated HNSW test setup into rstest fixtures:
create a #[fixture] that constructs HnswParams::new(...), calls
Graph::with_capacity(params, node_count), inserts the first node via
graph.insert_first(NodeContext { ... }) and attaches remaining nodes with
graph.attach_node(...), then have each test accept this fixture; replace
duplicated blocks around HnswParams, Graph::with_capacity, insert_first and
attach_node with parameterized #[rstest(...)] test cases that take the fixture
plus any per-case mutation data so tests 11-29, 51-64, 82-97, 118-138 reuse the
same setup and only supply differing inputs/assertions.
- Around line 31-34: The conditional inside the nested loops in the test uses a
redundant clause: drop the unnecessary "&& target < node_count" from the if
check that currently reads "if origin != target && target < node_count" because
the outer iteration already bounds target to 0..node_count; update the
conditional to just "if origin != target" in the loop that iterates origin,
target using node_count and referencing graph so the logic remains the same but
without the redundant check.
In `@chutoro-core/src/hnsw/tests/property/idempotency_property.rs`:
- Around line 153-163: The snapshot currently taken by snapshot_graph only
captures the graph's entry and nodes but misses the separate element count
stored on CpuHnsw; update the GraphSnapshot (or the snapshot_graph function) to
also capture index.len() (call index.len()) and include it in the returned
GraphSnapshot so tests compare the element count as well as topology, or
alternatively add an explicit assertion comparing index.len() before/after the
duplicate-insert loop; update references to GraphSnapshot and snapshot_graph
accordingly so the test will fail if len diverges from the structural snapshot.
In `@chutoro-core/src/hnsw/tests/property/mutation_property.rs`:
- Around line 202-209: The pool–index invariant is broken when
delete_node_for_test(node) returns Ok(false): the code currently returns
OperationOutcome::skipped without updating pools, leaving pools thinking the
node exists; update the Ok(false) arm to call self.pools.mark_deleted(node)
before returning the skipped outcome (or alternatively change the branch to
return a test failure if you prefer surfacing the inconsistency), referencing
the match on delete_node_for_test, the self.pools.mark_deleted(node) call, and
the OperationOutcome::skipped return so you update the proper branch.
In `@chutoro-core/src/mst/kani.rs`:
- Around line 37-43: In is_valid_forest, avoid indexing panics by validating
edge endpoints before calling kani_find_root: for each edge in edges, check that
edge.source() and edge.target() are < node_count (and optionally >= 0 if
applicable) and if either is out of bounds return false; only then call
kani_find_root(&mut parent, edge.source()) and kani_find_root(&mut parent,
edge.target()) and proceed with the union (parent[root_t] = root_s). This keeps
is_valid_forest total and prevents out-of-bounds access on parent and
kani_find_root.
- Around line 124-167: The harness never asserts MST minimality: when all three
edges are present and the result is connected, compute the total weight of the
returned MST (sum over forest.edges() weights) and assert it equals the minimal
possible spanning-tree weight for three nodes (sum of the two smallest of
f32::from(weight0), f32::from(weight1), f32::from(weight2)). Modify
verify_mst_minimality_3_nodes to, after obtaining forest from
parallel_kruskal_from_edges and checking connectivity (forest.component_count()
== 1), calculate expected_min = sum of the two smallest of
weight0/weight1/weight2 and assert total_mst_weight == expected_min (using exact
equality is fine because weights are converted from integer u8 to f32).
In `@chutoro-test-support/src/github_output.rs`:
- Around line 95-98: The loop currently opens the file first and writes each
entry directly (open_output_file + write_github_output_value), which can leave
GITHUB_OUTPUT partially mutated if a later write fails; instead, first encode
all entries into an in-memory buffer (e.g., a String or Vec<u8>) by calling
write_github_output_value against that buffer for every item in entries, and
only if all encodings succeed open the file with open_output_file and append the
complete buffer in one operation; update the code paths around entries, file,
open_output_file and write_github_output_value to implement this two-phase
(buffer-then-commit) approach and preserve existing error propagation.
- Around line 119-128: The split_output_path function currently accepts bare
filenames but needs a regression test to ensure parent paths that are empty are
normalised to "."; add a unit test named
split_output_path_normalises_bare_filename that constructs
PathBuf::from("github_output.txt"), calls
split_output_path(&output_path).expect(...), and asserts parent ==
Path::new(".") and file_name == OsStr::new("github_output.txt"); place the test
in the same module/tests section alongside other tests so it runs with cargo
test and prevents regressions in split_output_path's parent handling.
In `@chutoro-test-support/tests/benchmark_regression_gate_cli.rs`:
- Around line 162-173: The current code misuses Option.context() and checks
target/{profile} before deps; change the logic to first call
find_in_deps(&deps_dir)? and if it returns Some(path) immediately return that
path (map Utf8PathBuf::into_std_path_buf and Ok), otherwise construct the direct
path via with_exe_suffix(target_dir.join("benchmark_regression_gate")) and if
that exists return it, and if neither is found return an explicit error (use
anyhow::bail! or Err(anyhow!(...))). Update references: find_in_deps,
with_exe_suffix, target_dir, and the final error message to "failed to locate
benchmark_regression_gate binary".
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: 76b3b7c1-2438-42ed-ae3f-3db5260d2b90
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
Makefilechutoro-benches/src/fs_support.rschutoro-cli/src/cli/output.rschutoro-cli/src/logging.rschutoro-core/src/hnsw/cpu/build.rschutoro-core/src/hnsw/cpu/tests.rschutoro-core/src/hnsw/insert/commit/tests.rschutoro-core/src/hnsw/insert/commit/tests/eviction.rschutoro-core/src/hnsw/insert/commit/tests/healing.rschutoro-core/src/hnsw/insert/executor/tests/mod.rschutoro-core/src/hnsw/insert/test_helpers.rschutoro-core/src/hnsw/invariants/helpers.rschutoro-core/src/hnsw/invariants/tests/structural.rschutoro-core/src/hnsw/kani_proofs.rschutoro-core/src/hnsw/params.rschutoro-core/src/hnsw/tests/property/idempotency_property.rschutoro-core/src/hnsw/tests/property/mutation_property.rschutoro-core/src/mst/kani.rschutoro-core/src/result.rschutoro-providers/dense/src/ingest.rschutoro-providers/dense/src/provider.rschutoro-providers/dense/src/tests/errors.rschutoro-test-support/Cargo.tomlchutoro-test-support/src/github_output.rschutoro-test-support/tests/benchmark_regression_gate_cli.rsdocs/changelog.mddocs/migration-notes.mddocs/users-guide.md
Comment on lines
+98
to
+164
| fn validate_scoped_path(path: &Path) -> io::Result<()> { | ||
| if path | ||
| .components() | ||
| .any(|component| matches!(component, Component::ParentDir | Component::Prefix(_))) | ||
| { | ||
| return Err(invalid_scoped_path(path)); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn split_scope(path: &Path) -> io::Result<(Dir, PathBuf)> { | ||
| validate_scoped_path(path)?; | ||
| let root_path = if path.is_absolute() { | ||
| Path::new("/") | ||
| } else { | ||
| Path::new(".") | ||
| }; | ||
| let root_dir = Dir::open_ambient_dir(root_path, ambient_authority())?; | ||
| let mut relative = PathBuf::new(); | ||
| for component in path.components() { | ||
| match component { | ||
| Component::Normal(part) => relative.push(part), | ||
| Component::CurDir | ||
| | Component::RootDir | ||
| | Component::ParentDir | ||
| | Component::Prefix(_) => {} | ||
| } | ||
| } | ||
| Ok((root_dir, relative)) | ||
| } | ||
|
|
||
| fn open_scoped_dir(root_dir: Dir, relative_path: &Path, create_dir: bool) -> io::Result<Dir> { | ||
| if relative_path.as_os_str().is_empty() { | ||
| return Ok(root_dir); | ||
| } | ||
|
|
||
| if create_dir { | ||
| root_dir.create_dir_all(relative_path)?; | ||
| } | ||
|
|
||
| root_dir.open_dir(relative_path) | ||
| } | ||
|
|
||
| fn open_existing_parent_dir(path: &Path) -> io::Result<Option<Dir>> { | ||
| let parent = path.parent().unwrap_or_else(|| Path::new(".")); | ||
| let (root_dir, relative_parent) = split_scope(parent)?; | ||
| if relative_parent.as_os_str().is_empty() { | ||
| return Ok(Some(root_dir)); | ||
| } | ||
|
|
||
| match root_dir.open_dir(&relative_parent) { | ||
| Ok(dir) => Ok(Some(dir)), | ||
| Err(error) if error.kind() == io::ErrorKind::NotFound => Ok(None), | ||
| Err(error) => Err(error), | ||
| } | ||
| } | ||
|
|
||
| fn open_parent_dir(path: &Path, create_parent: bool) -> io::Result<(Dir, &OsStr)> { | ||
| validate_scoped_path(path)?; | ||
| let Some(file_name) = path.file_name() else { | ||
| return Err(invalid_path(path)); | ||
| }; | ||
| let parent = path.parent().unwrap_or_else(|| Path::new(".")); | ||
| let (root_dir, relative_parent) = split_scope(parent)?; | ||
| let dir = open_scoped_dir(root_dir, &relative_parent, create_parent)?; | ||
| Ok((dir, file_name)) | ||
| } |
Contributor
There was a problem hiding this comment.
Add direct regression tests for scoped-path and parent-resolution behaviour.
Add unit and behavioural tests for validate_scoped_path, split_scope, open_existing_parent_dir, and open_parent_dir. Cover .. rejection, root/empty parent handling, create-vs-open parent behaviour, and try_exists on missing parents.
As per coding guidelines, “Where a bug is being fixed, a unittest must be provided demonstrating the behaviour being corrected to validate the fix and guard against regression.” and “Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@chutoro-benches/src/fs_support.rs` around lines 98 - 164, Add unit and
integration tests that exercise validate_scoped_path, split_scope,
open_existing_parent_dir, and open_parent_dir to lock in expected scoped-path
and parent-resolution behavior: write tests that assert validate_scoped_path
rejects paths containing ParentDir or Prefix (e.g., ".." and Windows prefixes),
that split_scope returns correct (Dir, relative PathBuf) for absolute vs
relative inputs and treats root/empty parents properly, that
open_existing_parent_dir returns Ok(Some(dir)) for existing parents, Ok(None)
for missing parents (and that try_exists/metadata behave accordingly), and that
open_parent_dir creates parents only when create_parent=true and returns the
file_name via (Dir, &OsStr); place tests in the module’s test suite using
temporary dirs or cap-std ambient_dir helpers to avoid touching real FS and
include regression cases reproducing the bug so the tests fail before the fix
and pass after.
Comment on lines
+98
to
+115
| fn validate_scoped_path(path: &Path) -> io::Result<()> { | ||
| if path | ||
| .components() | ||
| .any(|component| matches!(component, Component::ParentDir | Component::Prefix(_))) | ||
| { | ||
| return Err(invalid_scoped_path(path)); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn split_scope(path: &Path) -> io::Result<(Dir, PathBuf)> { | ||
| validate_scoped_path(path)?; | ||
| let root_path = if path.is_absolute() { | ||
| Path::new("/") | ||
| } else { | ||
| Path::new(".") | ||
| }; | ||
| let root_dir = Dir::open_ambient_dir(root_path, ambient_authority())?; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Rust std::path on Windows, do absolute paths such as C:\tmp\file include Component::Prefix in Path::components, and would rejecting Component::Prefix therefore reject Windows absolute paths?
💡 Result:
Yes, in Rust's std::path on Windows, absolute paths such as C:\tmp\file include Component::Prefix as the first component in Path::components. The components are typically: Prefix("C:"), RootDir, Normal("tmp"), Normal("file"). Therefore, rejecting Component::Prefix would reject all Windows absolute paths that have a drive letter prefix, which is the standard form for absolute paths on Windows.
Citations:
- 1: https://doc.rust-lang.org/stable/std/path/enum.Component.html
- 2: https://doc.rust-lang.org/std/path/enum.Prefix.html
- 3: https://doc.rust-lang.org/std/path/struct.Path.html
- 4: https://doc.rust-lang.org/std/path/struct.PrefixComponent.html
- 5: https://users.rust-lang.org/t/understanding-windows-paths/58583
- 6: https://stackoverflow.com/questions/75103218/how-to-get-drive-letter-returned-as-a-string
🏁 Script executed:
cat chutoro-benches/src/fs_support.rs | head -100Repository: leynos/chutoro
Length of output: 3105
🏁 Script executed:
sed -n '98,125p' chutoro-benches/src/fs_support.rsRepository: leynos/chutoro
Length of output: 911
Fix Windows absolute-path support or narrow documented contract.
The module docs state that absolute paths are supported (line 4–7), but validate_scoped_path rejects all Component::Prefix(_) (line 101), which blocks all Windows absolute paths (e.g., C:\tmp) since they inherently begin with a drive-letter prefix. Either implement Windows prefix-aware path handling in split_scope or explicitly document platform-specific constraints and enforce them with cfg-gating.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@chutoro-benches/src/fs_support.rs` around lines 98 - 115, The code currently
rejects any Component::Prefix in validate_scoped_path, which prevents Windows
absolute paths (drive-letter prefixes) from being accepted by split_scope;
either (A) implement Windows-aware handling: in validate_scoped_path allow
Component::Prefix on windows (cfg(target_family = "windows")) and in split_scope
detect and preserve the prefix (e.g., extract the drive prefix and treat it as
the root component when computing root_path and opening Dir via
Dir::open_ambient_dir using the prefixed root) and ensure Path::is_absolute
behavior aligns with the prefix handling, or (B) narrow the documented contract
and cfg-gate the module: update docs to state absolute-path support is
POSIX-only and add cfg attributes so Component::Prefix is rejected only on
non-windows targets (leave validate_scoped_path as-is on non-windows). Ensure
you reference and modify validate_scoped_path and split_scope accordingly and
adjust tests/docs to reflect the chosen approach.
Comment on lines
+176
to
+193
| /// # Examples | ||
| /// ```rust,ignore | ||
| /// use chutoro_core::{CpuHnsw, DataSource, DataSourceError, HnswParams}; | ||
| /// # struct Dummy(Vec<f32>); | ||
| /// # impl DataSource for Dummy { | ||
| /// # fn len(&self) -> usize { self.0.len() } | ||
| /// # fn name(&self) -> &str { "dummy" } | ||
| /// # fn distance(&self, i: usize, j: usize) -> Result<f32, DataSourceError> { | ||
| /// # let a = self.0.get(i).ok_or(DataSourceError::OutOfBounds { index: i })?; | ||
| /// # let b = self.0.get(j).ok_or(DataSourceError::OutOfBounds { index: j })?; | ||
| /// # Ok((a - b).abs()) | ||
| /// # } | ||
| /// # } | ||
| /// let params = HnswParams::new(2, 4).expect("params"); | ||
| /// let data = Dummy(vec![0.0, 2.0, 4.0]); | ||
| /// let index = CpuHnsw::build(&data, params).expect("build must succeed"); | ||
| /// index.insert(1, &data).expect("insert must succeed"); | ||
| /// ``` |
Contributor
There was a problem hiding this comment.
Fix the insert Rustdoc example.
CpuHnsw::build(...) already inserts every item from the source, so the follow-on index.insert(1, &data) demonstrates duplicate insertion rather than incremental use. Start from an empty index, or another setup where node 1 has not already been inserted, so the docs match the actual API behaviour.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@chutoro-core/src/hnsw/cpu/build.rs` around lines 176 - 193, The example shows
duplicate insertion because CpuHnsw::build(...) already inserts all source
items; change the example to demonstrate incremental insertion by either
creating an empty index (use CpuHnsw::new(HnswParams) instead of
CpuHnsw::build(...)) and then calling insert(1, &data), or keep build(...) and
call insert with an id that wasn’t included in the source (e.g., insert(3,
&data)); update the doc example text accordingly so it uses CpuHnsw::new or a
non-duplicated id and matches the actual behavior of CpuHnsw::build and
CpuHnsw::insert.
Comment on lines
+219
to
+266
| let _insertion_guard = self | ||
| .insert_mutex | ||
| .lock() | ||
| .map_err(|_| HnswError::LockPoisoned { | ||
| resource: "insert mutex", | ||
| })?; | ||
| let level = self.sample_level()?; | ||
| let sequence = self.allocate_sequence(); | ||
| let node_ctx = NodeContext { | ||
| node, | ||
| level, | ||
| sequence, | ||
| }; | ||
| if self.try_insert_initial(node_ctx, source)? { | ||
| self.len.store(1, Ordering::Relaxed); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let cache = &self.distance_cache; | ||
| let plan = self.read_graph(|graph| { | ||
| graph.insertion_planner().plan(PlanningInputs { | ||
| ctx: node_ctx, | ||
| params: &self.params, | ||
| source, | ||
| cache: Some(cache), | ||
| }) | ||
| })?; | ||
|
|
||
| if let Some(edges) = edge_sink { | ||
| edges.extend(extract_candidate_edges(node, sequence, &plan)); | ||
| } | ||
|
|
||
| let (prepared, trim_jobs) = self.write_graph(|graph| { | ||
| let mut executor = graph.insertion_executor(); | ||
| executor.apply( | ||
| node_ctx, | ||
| ApplyContext { | ||
| params: &self.params, | ||
| plan, | ||
| }, | ||
| ) | ||
| })?; | ||
| let trim_results = self.score_trim_jobs(trim_jobs, source)?; | ||
| self.write_graph(|graph| { | ||
| let mut executor = graph.insertion_executor(); | ||
| executor.commit(prepared, trim_results) | ||
| })?; | ||
| self.len.fetch_add(1, Ordering::Relaxed); |
Contributor
There was a problem hiding this comment.
Restore actual parallelism in the build path.
Hold insert_mutex only around the shared mutation, or replace the Rayon loop with a plain sequential loop. The current guard spans planning, trim scoring, and commit, so every task spawned by build() and build_with_edges() blocks on the same mutex and runs serially. That turns the new parallel builders into a contention-heavy serial loop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@chutoro-core/src/hnsw/cpu/build.rs` around lines 219 - 266, The insert_mutex
is held across planning, scoring and commit which serializes all build tasks;
instead acquire the lock only for the minimal shared-mutation sections (e.g.
around allocate_sequence(), the initial insertion check
try_insert_initial(node_ctx, ...), and the final len updates), then drop it
before calling sample_level(), read_graph(...) planning, score_trim_jobs(...),
and write_graph(...) work; specifically, move the lock so it only protects the
sequence allocation/initial-insert path and the len.fetch_add/fetch_store
updates, allowing read_graph, extract_candidate_edges, score_trim_jobs, and
write_graph executor.apply/commit to run without the long-lived insert_mutex.
Comment on lines
+15
to
+54
| #[test] | ||
| fn insert_waits_for_mutex() { | ||
| let params = HnswParams::new(2, 4).expect("params").with_rng_seed(31); | ||
| let index = Arc::new(CpuHnsw::with_capacity(params, 2).expect("index")); | ||
| let source = Arc::new(TestSource::new(vec![0.0, 1.0])); | ||
|
|
||
| let guard = index.insert_mutex.lock().expect("mutex"); | ||
| let started = Arc::new(AtomicBool::new(false)); | ||
| let finished = Arc::new(AtomicBool::new(false)); | ||
| let (started_tx, started_rx) = mpsc::channel(); | ||
|
|
||
| let handle = { | ||
| let index = Arc::clone(&index); | ||
| let source = Arc::clone(&source); | ||
| let started = Arc::clone(&started); | ||
| let finished = Arc::clone(&finished); | ||
| let started_tx = started_tx.clone(); | ||
| thread::spawn(move || { | ||
| started.store(true, AtomicOrdering::SeqCst); | ||
| started_tx | ||
| .send(()) | ||
| .expect("worker must signal that insert is about to start"); | ||
| index.insert(0, &*source).expect("insert must succeed"); | ||
| finished.store(true, AtomicOrdering::SeqCst); | ||
| }) | ||
| }; | ||
|
|
||
| started_rx | ||
| .recv_timeout(Duration::from_secs(1)) | ||
| .expect("worker must start within one second"); | ||
| assert!(started.load(AtomicOrdering::SeqCst)); | ||
| assert!( | ||
| !finished.load(AtomicOrdering::SeqCst), | ||
| "insert should block while the mutex is held" | ||
| ); | ||
|
|
||
| drop(guard); | ||
| handle.join().expect("thread joins"); | ||
| assert!(finished.load(AtomicOrdering::SeqCst)); | ||
| } |
Contributor
There was a problem hiding this comment.
Add a poisoned-lock regression test.
Exercise a poisoned insert_mutex and assert HnswError::LockPoisoned { resource: "insert mutex" }. This file only covers waiting on the mutex, but the panic-to-error path added in chutoro-core/src/hnsw/cpu/build.rs still has no regression coverage.
As per coding guidelines, "Where a bug is being fixed, a unittest must be provided demonstrating the behaviour being corrected to validate the fix and guard against regression."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@chutoro-core/src/hnsw/cpu/tests.rs` around lines 15 - 54, Add a regression
test that exercises a poisoned insert_mutex and asserts the insert returns
HnswError::LockPoisoned { resource: "insert mutex" }: create a test similar to
insert_waits_for_mutex which spawns a thread that locks index.insert_mutex and
then panics (to poison the lock), join/ignore the panicking thread, then call
index.insert(...) and assert the returned Err matches HnswError::LockPoisoned
with resource "insert mutex" (use CpuHnsw::insert and the HnswError enum to
match the error); ensure the test uses the same TestSource/HnswParams setup and
Atomic flags to coordinate if needed so the lock is poisoned before calling
insert.
Comment on lines
+37
to
+43
| for edge in edges { | ||
| let root_s = kani_find_root(&mut parent, edge.source()); | ||
| let root_t = kani_find_root(&mut parent, edge.target()); | ||
| if root_s == root_t { | ||
| return false; | ||
| } | ||
| parent[root_t] = root_s; |
Contributor
There was a problem hiding this comment.
Prevent panic paths in is_valid_forest by validating endpoint bounds first.
Guard edge.source() and edge.target() against node_count before calling kani_find_root. Keep this validator total and return false for malformed forests instead of indexing out of bounds on Line 38 and Line 39.
Patch
let mut parent: Vec<usize> = (0..node_count).collect();
for edge in edges {
- let root_s = kani_find_root(&mut parent, edge.source());
- let root_t = kani_find_root(&mut parent, edge.target());
+ let source = edge.source();
+ let target = edge.target();
+ if source >= node_count || target >= node_count {
+ return false;
+ }
+
+ let root_s = kani_find_root(&mut parent, source);
+ let root_t = kani_find_root(&mut parent, target);
if root_s == root_t {
return false;
}
parent[root_t] = root_s;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for edge in edges { | |
| let root_s = kani_find_root(&mut parent, edge.source()); | |
| let root_t = kani_find_root(&mut parent, edge.target()); | |
| if root_s == root_t { | |
| return false; | |
| } | |
| parent[root_t] = root_s; | |
| for edge in edges { | |
| let source = edge.source(); | |
| let target = edge.target(); | |
| if source >= node_count || target >= node_count { | |
| return false; | |
| } | |
| let root_s = kani_find_root(&mut parent, source); | |
| let root_t = kani_find_root(&mut parent, target); | |
| if root_s == root_t { | |
| return false; | |
| } | |
| parent[root_t] = root_s; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@chutoro-core/src/mst/kani.rs` around lines 37 - 43, In is_valid_forest, avoid
indexing panics by validating edge endpoints before calling kani_find_root: for
each edge in edges, check that edge.source() and edge.target() are < node_count
(and optionally >= 0 if applicable) and if either is out of bounds return false;
only then call kani_find_root(&mut parent, edge.source()) and
kani_find_root(&mut parent, edge.target()) and proceed with the union
(parent[root_t] = root_s). This keeps is_valid_forest total and prevents
out-of-bounds access on parent and kani_find_root.
Comment on lines
+124
to
+167
| /// Verifies MST minimality property for bounded graphs. | ||
| /// | ||
| /// This harness verifies that the MST includes minimum weight edges by | ||
| /// checking that the total weight is minimal. For a 3-node graph, if | ||
| /// all edges are present, the MST must exclude the heaviest edge that | ||
| /// would create a cycle. | ||
| #[kani::proof] | ||
| #[kani::unwind(10)] | ||
| fn verify_mst_minimality_3_nodes() { | ||
| let node_count = 3usize; | ||
| let mut edges = Vec::new(); | ||
|
|
||
| let weight0: u8 = kani::any(); | ||
| let weight1: u8 = kani::any(); | ||
| let weight2: u8 = kani::any(); | ||
|
|
||
| if kani::any::<bool>() { | ||
| edges.push(CandidateEdge::new(0, 1, f32::from(weight0), 0)); | ||
| } | ||
| if kani::any::<bool>() { | ||
| edges.push(CandidateEdge::new(1, 2, f32::from(weight1), 1)); | ||
| } | ||
| if kani::any::<bool>() { | ||
| edges.push(CandidateEdge::new(0, 2, f32::from(weight2), 2)); | ||
| } | ||
|
|
||
| let Ok(forest) = parallel_kruskal_from_edges(node_count, edges.iter()) else { | ||
| return; | ||
| }; | ||
|
|
||
| let mst_edges = forest.edges(); | ||
|
|
||
| kani::assert( | ||
| is_valid_forest(node_count, mst_edges, forest.component_count()), | ||
| "MST forest invariant violated", | ||
| ); | ||
|
|
||
| if forest.component_count() == 1 { | ||
| kani::assert( | ||
| mst_edges.len() == node_count.saturating_sub(1), | ||
| "connected MST should have n-1 edges", | ||
| ); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Prove minimality explicitly in verify_mst_minimality_3_nodes.
Assert the minimal total weight when all three edges exist. The current harness name and docs promise minimality, but Lines 156-165 only assert structural properties.
Patch
fn verify_mst_minimality_3_nodes() {
let node_count = 3usize;
let mut edges = Vec::new();
let weight0: u8 = kani::any();
let weight1: u8 = kani::any();
let weight2: u8 = kani::any();
- if kani::any::<bool>() {
+ let include_01: bool = kani::any();
+ let include_12: bool = kani::any();
+ let include_02: bool = kani::any();
+
+ if include_01 {
edges.push(CandidateEdge::new(0, 1, f32::from(weight0), 0));
}
- if kani::any::<bool>() {
+ if include_12 {
edges.push(CandidateEdge::new(1, 2, f32::from(weight1), 1));
}
- if kani::any::<bool>() {
+ if include_02 {
edges.push(CandidateEdge::new(0, 2, f32::from(weight2), 2));
}
let Ok(forest) = parallel_kruskal_from_edges(node_count, edges.iter()) else {
return;
@@
if forest.component_count() == 1 {
kani::assert(
mst_edges.len() == node_count.saturating_sub(1),
"connected MST should have n-1 edges",
);
}
+
+ if include_01 && include_12 && include_02 && forest.component_count() == 1 {
+ let mst_weight: f32 = mst_edges.iter().map(|edge| edge.weight()).sum();
+ let w0 = f32::from(weight0);
+ let w1 = f32::from(weight1);
+ let w2 = f32::from(weight2);
+ let expected = (w0 + w1 + w2) - w0.max(w1).max(w2);
+ kani::assert(mst_weight == expected, "MST did not minimize total weight");
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@chutoro-core/src/mst/kani.rs` around lines 124 - 167, The harness never
asserts MST minimality: when all three edges are present and the result is
connected, compute the total weight of the returned MST (sum over forest.edges()
weights) and assert it equals the minimal possible spanning-tree weight for
three nodes (sum of the two smallest of f32::from(weight0), f32::from(weight1),
f32::from(weight2)). Modify verify_mst_minimality_3_nodes to, after obtaining
forest from parallel_kruskal_from_edges and checking connectivity
(forest.component_count() == 1), calculate expected_min = sum of the two
smallest of weight0/weight1/weight2 and assert total_mst_weight == expected_min
(using exact equality is fine because weights are converted from integer u8 to
f32).
Comment on lines
+95
to
+98
| let mut file = open_output_file(Path::new(&output_path))?; | ||
| for entry in entries { | ||
| write_github_output_value(&mut file, entry.key(), entry.value())?; | ||
| } |
Contributor
There was a problem hiding this comment.
Validate the batch before appending.
Buffer the encoded entries before opening the file. Line 97 can fail after earlier pairs have already been appended, so callers receive an error and a partially mutated GITHUB_OUTPUT file.
🩹 Proposed fix
pub fn append_if_configured(entries: &[OutputPair<'_>]) -> io::Result<()> {
let Some(output_path) = read_optional_env("GITHUB_OUTPUT")? else {
return Ok(());
};
if output_path.is_empty() {
return Ok(());
}
+ let mut buffer = Vec::new();
+ for entry in entries {
+ write_github_output_value(&mut buffer, entry.key(), entry.value())?;
+ }
+
let mut file = open_output_file(Path::new(&output_path))?;
- for entry in entries {
- write_github_output_value(&mut file, entry.key(), entry.value())?;
- }
+ file.write_all(&buffer)?;
Ok(())
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@chutoro-test-support/src/github_output.rs` around lines 95 - 98, The loop
currently opens the file first and writes each entry directly (open_output_file
+ write_github_output_value), which can leave GITHUB_OUTPUT partially mutated if
a later write fails; instead, first encode all entries into an in-memory buffer
(e.g., a String or Vec<u8>) by calling write_github_output_value against that
buffer for every item in entries, and only if all encodings succeed open the
file with open_output_file and append the complete buffer in one operation;
update the code paths around entries, file, open_output_file and
write_github_output_value to implement this two-phase (buffer-then-commit)
approach and preserve existing error propagation.
Comment on lines
+119
to
+128
| fn split_output_path(output_path: &Path) -> io::Result<(&Path, &OsStr)> { | ||
| let parent = output_path.parent().unwrap_or_else(|| Path::new(".")); | ||
| let Some(file_name) = output_path.file_name() else { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidInput, | ||
| format!("invalid GITHUB_OUTPUT path: {}", output_path.display()), | ||
| )); | ||
| }; | ||
|
|
||
| Ok((parent, file_name)) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, let me examine the file structure and read the relevant file
cd chutoro-test-support && wc -l src/github_output.rsRepository: leynos/chutoro
Length of output: 82
🏁 Script executed:
# Read the entire file to understand context
cat -n src/github_output.rsRepository: leynos/chutoro
Length of output: 110
🏁 Script executed:
# Search for tests related to split_output_path or github_output
rg -n "split_output_path|github_output" chutoro-test-support/src/Repository: leynos/chutoro
Length of output: 4371
🏁 Script executed:
# Verify the Path::parent() behaviour with a quick test
python3 << 'EOF'
from pathlib import Path
# Test what parent() returns for a bare filename
test_cases = [
"github_output.txt",
"./github_output.txt",
"output/github_output.txt",
"/tmp/github_output.txt"
]
for test_path in test_cases:
p = Path(test_path)
parent = p.parent
print(f"Path: '{test_path}'")
print(f" parent: '{parent}' (parts: {parent.parts})")
print(f" is_empty-like: {str(parent) == ''}")
print()
EOFRepository: leynos/chutoro
Length of output: 402
🏁 Script executed:
# Read the github_output.rs file from the correct path
cat -n chutoro-test-support/src/github_output.rsRepository: leynos/chutoro
Length of output: 12963
Add regression test for bare filename input.
The current implementation accepts bare filenames (e.g., github_output.txt) but Path::parent() yields an empty path rather than None, which differs from the current directory representation. The proposed fix correctly filters empty paths to normalise them to .. However, per the coding guidelines, a regression test demonstrating this behaviour must be added to prevent regression.
Bare filename test case
Add a test that verifies split_output_path handles bare filenames correctly:
#[test]
fn split_output_path_normalises_bare_filename() {
let output_path = PathBuf::from("github_output.txt");
let (parent, file_name) =
split_output_path(&output_path).expect("path splitting must succeed");
assert_eq!(parent, Path::new("."));
assert_eq!(file_name, std::ffi::OsStr::new("github_output.txt"));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@chutoro-test-support/src/github_output.rs` around lines 119 - 128, The
split_output_path function currently accepts bare filenames but needs a
regression test to ensure parent paths that are empty are normalised to "."; add
a unit test named split_output_path_normalises_bare_filename that constructs
PathBuf::from("github_output.txt"), calls
split_output_path(&output_path).expect(...), and asserts parent ==
Path::new(".") and file_name == OsStr::new("github_output.txt"); place the test
in the same module/tests section alongside other tests so it runs with cargo
test and prevents regressions in split_output_path's parent handling.
Comment on lines
162
to
+173
| let target_dir = deps_dir | ||
| .parent() | ||
| .map(Path::to_path_buf) | ||
| .ok_or_else(|| "failed to resolve target directory from deps".to_string())?; | ||
| .map(Utf8Path::to_path_buf) | ||
| .context("failed to resolve target directory from deps")?; | ||
| let direct = with_exe_suffix(target_dir.join("benchmark_regression_gate")); | ||
| if direct.exists() { | ||
| return Ok(direct); | ||
| return Ok(direct.into_std_path_buf()); | ||
| } | ||
|
|
||
| find_in_deps(&deps_dir) | ||
| .ok_or_else(|| "failed to locate benchmark_regression_gate binary".to_string()) | ||
| find_in_deps(&deps_dir)? | ||
| .map(Utf8PathBuf::into_std_path_buf) | ||
| .context("failed to locate benchmark_regression_gate binary") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "benchmark_regression_gate_cli.rs" -type fRepository: leynos/chutoro
Length of output: 119
🏁 Script executed:
head -n 200 chutoro-test-support/tests/benchmark_regression_gate_cli.rs | tail -n 50Repository: leynos/chutoro
Length of output: 2017
🏁 Script executed:
cat -n chutoro-test-support/tests/benchmark_regression_gate_cli.rs | sed -n '140,200p'Repository: leynos/chutoro
Length of output: 2685
🏁 Script executed:
cat -n chutoro-test-support/tests/benchmark_regression_gate_cli.rs | sed -n '166,174p'Repository: leynos/chutoro
Length of output: 425
🏁 Script executed:
# Check if the code compiles as written by looking for any error handling patterns
rg "find_in_deps" chutoro-test-support/tests/benchmark_regression_gate_cli.rs -A 5Repository: leynos/chutoro
Length of output: 563
🏁 Script executed:
# Understand the actual problem: does the return type chain correctly?
# Check the full return type of binary_path()
cat -n chutoro-test-support/tests/benchmark_regression_gate_cli.rs | sed -n '150,174p'Repository: leynos/chutoro
Length of output: 1307
🏁 Script executed:
# Check if there's a type mismatch: does Option have a context() method?
# Let's examine the exact type chain at lines 171-173
rg -A 3 "find_in_deps.*\?" chutoro-test-support/tests/benchmark_regression_gate_cli.rsRepository: leynos/chutoro
Length of output: 203
🏁 Script executed:
# Check imports to see if there's an extension trait for Option
head -n 30 chutoro-test-support/tests/benchmark_regression_gate_cli.rsRepository: leynos/chutoro
Length of output: 767
🏁 Script executed:
# Verify if the code actually compiles by checking for build artifacts
cd chutoro-test-support && cargo check --tests 2>&1 | head -n 50Repository: leynos/chutoro
Length of output: 7000
🏁 Script executed:
# Check if there's a custom extension on Option or if Context is extended
rg "impl.*Context.*Option\|trait.*Context" chutoro-test-support/tests/benchmark_regression_gate_cli.rsRepository: leynos/chutoro
Length of output: 40
🏁 Script executed:
# Check the exact type at that point - look for any wrapper or conversion
rg -B 5 "find_in_deps" chutoro-test-support/tests/benchmark_regression_gate_cli.rs | head -n 30Repository: leynos/chutoro
Length of output: 441
🏁 Script executed:
# Let's look at the actual full file to spot any type conversions or custom traits
wc -l chutoro-test-support/tests/benchmark_regression_gate_cli.rsRepository: leynos/chutoro
Length of output: 121
🏁 Script executed:
cat chutoro-test-support/tests/benchmark_regression_gate_cli.rsRepository: leynos/chutoro
Length of output: 7952
🏁 Script executed:
# Confirm the anyhow Context trait supports Option
rg "impl.*Context.*Option" ~/.cargo/registry/src/ 2>/dev/null || echo "Checking anyhow crate docs approach instead"Repository: leynos/chutoro
Length of output: 101
🏁 Script executed:
# Understand Cargo's binary naming in deps:
# The hashed binaries in deps are created fresh during each build
# Let's verify the binary resolution strategy is indeed problematic
# Check if there are any comments or docs explaining the fallback logic
rg -B 5 "direct.exists" chutoro-test-support/tests/benchmark_regression_gate_cli.rsRepository: leynos/chutoro
Length of output: 313
🏁 Script executed:
# Verify the actual build artifact locations by examining how Cargo typically organizes them
# Check if there are related build scripts or Cargo.toml that might shed light on intended behaviour
find chutoro-test-support -name "Cargo.toml" -exec head -n 30 {} \;Repository: leynos/chutoro
Length of output: 432
Fix compilation error and search deps before target/{profile}.
Lines 171–173 fail to compile: Option<PathBuf>.context() is not valid (.context() extends only Result, not Option). Simultaneously, the fallback order is unsound. When CARGO_BIN_EXE_benchmark_regression_gate is absent, the code returns the direct path in target_dir if it exists, which may be a stale artefact from an earlier cargo build. The hashed binary in deps is fresh and created during the current build's linking phase. Reorder to search deps first, then fall back to the direct path if needed, and explicitly return an error if neither is found.
🩹 Proposed fix
- let direct = with_exe_suffix(target_dir.join("benchmark_regression_gate"));
- if direct.exists() {
- return Ok(direct.into_std_path_buf());
- }
-
- find_in_deps(&deps_dir)?
- .map(Utf8PathBuf::into_std_path_buf)
- .context("failed to locate benchmark_regression_gate binary")
+ if let Some(path) = find_in_deps(&deps_dir)? {
+ return Ok(path.into_std_path_buf());
+ }
+
+ let direct = with_exe_suffix(target_dir.join("benchmark_regression_gate"));
+ if direct.exists() {
+ return Ok(direct.into_std_path_buf());
+ }
+
+ Err(anyhow::anyhow!(
+ "failed to locate benchmark_regression_gate binary"
+ ))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let target_dir = deps_dir | |
| .parent() | |
| .map(Path::to_path_buf) | |
| .ok_or_else(|| "failed to resolve target directory from deps".to_string())?; | |
| .map(Utf8Path::to_path_buf) | |
| .context("failed to resolve target directory from deps")?; | |
| let direct = with_exe_suffix(target_dir.join("benchmark_regression_gate")); | |
| if direct.exists() { | |
| return Ok(direct); | |
| return Ok(direct.into_std_path_buf()); | |
| } | |
| find_in_deps(&deps_dir) | |
| .ok_or_else(|| "failed to locate benchmark_regression_gate binary".to_string()) | |
| find_in_deps(&deps_dir)? | |
| .map(Utf8PathBuf::into_std_path_buf) | |
| .context("failed to locate benchmark_regression_gate binary") | |
| let target_dir = deps_dir | |
| .parent() | |
| .map(Utf8Path::to_path_buf) | |
| .context("failed to resolve target directory from deps")?; | |
| if let Some(path) = find_in_deps(&deps_dir)? { | |
| return Ok(path.into_std_path_buf()); | |
| } | |
| let direct = with_exe_suffix(target_dir.join("benchmark_regression_gate")); | |
| if direct.exists() { | |
| return Ok(direct.into_std_path_buf()); | |
| } | |
| Err(anyhow::anyhow!( | |
| "failed to locate benchmark_regression_gate binary" | |
| )) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@chutoro-test-support/tests/benchmark_regression_gate_cli.rs` around lines 162
- 173, The current code misuses Option.context() and checks target/{profile}
before deps; change the logic to first call find_in_deps(&deps_dir)? and if it
returns Some(path) immediately return that path (map
Utf8PathBuf::into_std_path_buf and Ok), otherwise construct the direct path via
with_exe_suffix(target_dir.join("benchmark_regression_gate")) and if that exists
return it, and if neither is found return an explicit error (use anyhow::bail!
or Err(anyhow!(...))). Update references: find_in_deps, with_exe_suffix,
target_dir, and the final error message to "failed to locate
benchmark_regression_gate binary".
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 by Sourcery
Enable Whitaker linting and tighten safety/error handling across the core, CLI, benches, and test-support crates while preserving existing behaviour.
New Features:
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests: