Conversation
📝 WalkthroughWalkthroughLarge repo-wide formatting plus targeted feature and test additions: sandboxed tool-output wrapping, path-resolution and grep/list tooling with budgets, a new public Calibrator API, ContextMisleading feedback verdict and recorder, hybrid BM25+embedding (RRF) retrieval paths, richer context-enrichment with TTL'd caches, hydration improvements, and extensive test coverage across many modules. ChangesAgent / Tooling / Feedback / Calibrator Cohort
Retrieval / Indexing / Enrichment Cohort
Hydration / Extraction / Analysis Cohort
Repository-wide Formatting & Minor Refactors Cohort
Sequence Diagram(s)sequenceDiagram
participant Client
participant Pipeline
participant Retriever
participant Embedder
participant BM25
participant FeedbackIndex
Client->>Pipeline: submit query/review request
Pipeline->>Retriever: build RetrievalQuery (identifiers, structural_names, text)
Retriever->>FeedbackIndex: fetch BM25 hits
Retriever->>Embedder: (if embedder init) compute query embedding
Embedder-->>Retriever: query vector
Retriever->>FeedbackIndex: fetch vector-ranked hits
FeedbackIndex->>FeedbackIndex: apply RRF fusion (BM25 + embedding similarities)
FeedbackIndex-->>Retriever: fused results
Retriever-->>Pipeline: ranked chunks
Pipeline->>Client: rendered injection / review output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/domain.rs (2)
155-172:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winESPHome detection currently skips
.ymlconfigsLine 155 and Line 171 only allow
yaml. Repos using*.ymlwill be missed.Suggested patch
- if path.extension().and_then(|e| e.to_str()) == Some("yaml") { + if matches!(path.extension().and_then(|e| e.to_str()), Some("yaml" | "yml")) { @@ - if path.extension().and_then(|e| e.to_str()) == Some("yaml") { + if matches!(path.extension().and_then(|e| e.to_str()), Some("yaml" | "yml")) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain.rs` around lines 155 - 172, The ESPHome detection only checks for "yaml" extensions and thus misses ".yml" files; update both places that use path.extension().and_then(|e| e.to_str()) == Some("yaml") (the blocks that read file contents and then check content.starts_with("esphome:") / contains("\nesphome:")) to accept either "yaml" or "yml" (e.g., compare a lowercased extension against "yaml" || "yml" or match both strings) so repositories with *.yml are detected as well.
157-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFunctional logic changed in a formatting-only PR scope
Line 157 and Line 173 change ESPHome detection behavior. This is not formatting, so it should be split into a separate PR (or the PR objective/description should be updated) to keep review scope and blame history clean.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain.rs` around lines 157 - 173, The PR accidentally changed ESPHome detection logic (the checks that insert "esphome" into the frameworks set) — revert the behavioral change or move it out of this formatting PR: restore the original conditional(s) that inspect file contents for ESPhome (the content.starts_with("esphome:") and content.contains("\nesphome:") checks used when iterating entries under project_dir and for root-level YAML files that update the frameworks set), or if the behavior change is intended, put only that change into a separate functional PR (and update this PR description) so formatting and logic remain separate.src/feedback.rs (1)
260-267:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
try_exists()here so non-race I/O failures still propagate.
Path::exists()collapses stat errors intofalse, so this branch can silently downgrade a real follow-up I/O/permission failure intoOk(false). That breaks the helper's stated contract of only swallowing the benign "source already vanished" race.Suggested fix
- Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { // Issue `#101`: NotFound is only the "another process already // moved it" race signal when the source has actually vanished. // If src is still present, the NotFound came from somewhere // else (missing dst parent, missing intermediate dir, etc.) — // propagate so the misconfiguration surfaces. - if src.exists() { Err(e) } else { Ok(false) } + match src.try_exists() { + Ok(true) => Err(e), + Ok(false) => Ok(false), + Err(stat_err) => Err(stat_err), + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/feedback.rs` around lines 260 - 267, Replace the src.exists() check with Path::try_exists() so I/O/stat errors are not collapsed into false; in the Err(e) if e.kind() == std::io::ErrorKind::NotFound branch, call src.try_exists(), propagate any Err returned from try_exists() (return Err(err) rather than treating it as Ok(false)), and only treat Ok(false) as Ok(false) while treating Ok(true) as Err(e) to preserve the original race semantics.src/agent.rs (1)
377-380:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop executing later tool calls once the budget is exhausted mid-batch.
A truncating call can set
total_bytes_readto the cap, but this loop still runs the rest of the tool calls from the same model response becauselimit_reached()is only checked after the batch. That defeats the hard investigation budget and can still do extra tool work before the forced-final turn.Suggested fix
for tc in calls { match state.execute_tool_call(&tc, tools, config) { Some(r) => executed.push((tc, r)), None => break, } + if state.limit_reached(config) { + break; + } }Also applies to: 405-411
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent.rs` around lines 377 - 380, The loop over "calls" keeps executing tool calls even if the investigation budget is exhausted mid-batch; after each successful call via state.execute_tool_call(&tc, tools, config) (where you push to executed), immediately check state.limit_reached() (or whether total_bytes_read hit the cap) and break out of the loop if true to prevent further tool work; apply the same fix to the other identical loop using execute_tool_call at the later block (the 405-411 occurrence).src/feedback_index.rs (1)
62-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winShort-circuit empty stores before loading fastembed.
build_hybrid()andbuild()still callLocalEmbedder::new()whenentries.is_empty(). On a fresh feedback store that pays the full model-init cost for an index with no vectors, andbuild()can returnembedder: Some(_)state thatfind_similar()will never use. Return early withembedder: Nonewhen there are no entries.Suggested guard for the default build path (mirror the same guard in
build_hybrid())pub fn build(store: &FeedbackStore) -> anyhow::Result<Self> { let entries = store.load_all()?; + if entries.is_empty() { + return Ok(Self { + entries, + #[cfg(feature = "embeddings")] + vectors: vec![], + #[cfg(feature = "embeddings")] + embedder: None, + bm25_engine: None, + }); + } // BM25 is always built alongside (unless disabled or store is empty) // so that RRF fusion works even when embeddings fail to initialize — // and so callers without embeddings fall back to BM25, not Jaccard.Also applies to: 161-233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/feedback_index.rs` around lines 62 - 80, build_hybrid (and build) currently calls LocalEmbedder::new() even when entries.is_empty(), causing unnecessary model init and returning embedder: Some(_) that find_similar() never uses; update build_hybrid and build to check entries.is_empty() immediately after loading the store and either return early with embedder: None (or set embedder = None) before calling LocalEmbedder::new(), so LocalEmbedder::new() is only invoked when there are entries to embed; ensure the same guard is applied for the default build path and references to find_similar remain consistent with embedder being optional.src/calibrator.rs (2)
1087-1091:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCount each verdict at most once per chunk id.
blamed_chunk_idsis treated as a raw list here, so one malformed feedback row like["chunk-a", "chunk-a"]records two confirmations and can seal a chunk early. A singleContextMisleadingverdict should contribute only one confirmation per chunk.Suggested change
-use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; @@ for entry in feedback { if let Verdict::ContextMisleading { blamed_chunk_ids } = &entry.verdict { - for chunk_id in blamed_chunk_ids { - *cal.misleading_counts.entry(chunk_id.clone()).or_insert(0) += 1; + let unique_ids: HashSet<&str> = + blamed_chunk_ids.iter().map(String::as_str).collect(); + for chunk_id in unique_ids { + *cal.misleading_counts + .entry(chunk_id.to_string()) + .or_insert(0) += 1; } } }As per coding guidelines,
**/calibrator.rs: When feedback verdict iscontext_misleadingwithblamed_chunks, raise per-chunk injection thresholds viaCalibrator::injection_threshold_for.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/calibrator.rs` around lines 1087 - 1091, The loop handling Verdict::ContextMisleading is counting duplicate chunk ids from a single feedback entry multiple times; change it to deduplicate blamed_chunk_ids per entry (e.g., collect into a local HashSet) and then increment cal.misleading_counts once per unique chunk_id so a malformed row like ["chunk-a","chunk-a"] only yields one confirmation; ensure you update the logic around Verdict::ContextMisleading and use Calibrator::injection_threshold_for as before (the place to modify is the block that references cal.misleading_counts and blamed_chunk_ids).
1073-1079:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThread the configured
inject_suppress_afterintoCalibratorconstruction.
Calibrator::new()hardcodes the default suppression budget of 3. The production-like call pattern insrc/context/phase6_integration_tests.rs:422-426only passesinject_min_score, so any non-defaultinject_suppress_aftersetting will still escalate and seal chunks on the wrong confirmation count.Suggested change
- pub fn new(inject_floor: f32) -> Self { - let defaults = CalibratorConfig::default(); + pub fn new(inject_floor: f32, inject_suppress_after: u32) -> Self { Self { inject_floor, - inject_suppress_after: defaults.inject_suppress_after, + inject_suppress_after: inject_suppress_after.max(1), misleading_counts: HashMap::new(), } } @@ - pub fn from_feedback(inject_floor: f32, feedback: &[FeedbackEntry]) -> Self { - let mut cal = Self::new(inject_floor); + pub fn from_feedback( + inject_floor: f32, + inject_suppress_after: u32, + feedback: &[FeedbackEntry], + ) -> Self { + let mut cal = Self::new(inject_floor, inject_suppress_after);As per coding guidelines,
**/calibrator.rs: Permanently seal chunks (set threshold to f32::INFINITY) afterinject_suppress_afterconfirmations of context_misleading verdict.Also applies to: 1097-1100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/calibrator.rs` around lines 1073 - 1079, Calibrator::new currently ignores any non-default inject_suppress_after and always uses CalibratorConfig::default(); change the constructor to accept and store the configured suppress budget (e.g., add an inject_suppress_after parameter or accept a CalibratorConfig) so the field inject_suppress_after on Calibrator is initialized from the passed configuration rather than defaults; update the other constructor/creation site referenced around the second occurrence (symbols: Calibrator::new, CalibratorConfig, inject_suppress_after, misleading_counts) and ensure the sealing logic that sets thresholds to f32::INFINITY uses this stored inject_suppress_after to seal after that many context_misleading confirmations.src/context/phase6_integration_tests.rs (1)
115-120: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd one end-to-end case that populates
structural_names.Every updated
InjectionRequesthere still hard-codesstructural_namestovec![], so the new exact-match retrieval leg never runs in this suite. A dedicated case with a real structural name would pin the pipeline→injector contract and catch regressions where those names stop being forwarded or normalized correctly.Also applies to: 151-156, 175-180, 199-204, 224-229, 257-262, 387-392
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/context/phase6_integration_tests.rs` around lines 115 - 120, Tests in phase6_integration_tests.rs never exercise the structural-name exact-match path because every InjectionRequest sets structural_names to vec![]; add at least one end-to-end test case that supplies a real structural name so the pipeline→injector contract is exercised. Modify the InjectionRequest instances (the struct InjectionRequest used in the tests) to include a structural_names entry matching the symbol under test (e.g., for file_path "src/auth.rs" and identifier "verify_token" set structural_names: vec!["auth::verify_token".to_string()] or vec!["verify_token".to_string()]), add a dedicated case that asserts the expected retrieval/injection behavior, and repeat this change for the other listed test blocks so the exact-match retrieval leg runs. Ensure you use the same normalization/format for structural names the injector expects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/context_enrichment.rs`:
- Around line 613-617: The log call that prints "Context7 query_docs" slices the
HTTP response body by bytes and can panic on non-UTF-8/partial-UTF-8 boundaries;
update the logging in the Context7/ query_docs path (the eprintln! with
"Context7 query_docs: HTTP {} - {}") to produce a safe truncated string instead
of &body[..200.min(body.len())] — for example use a UTF-8-safe truncation like
body.get(..200).unwrap_or(&body) or body.chars().take(200).collect::<String>()
so the logger never panics on non-ASCII or multi-byte characters.
In `@src/context/cli.rs`:
- Around line 1950-1952: The code is swallowing errors from
deps.git().head_sha(root) by using unwrap_or(None), causing doctor --repair to
appear successful while writing an invalid state.json; change the head_sha
extraction in the source_repo_root match so that errors from deps.git().head_sha
propagate into the existing best-effort warning path instead of being converted
to None: replace the unwrap_or(None) use on deps.git().head_sha(root) with logic
that returns or forwards the Err (or maps it into the same warning-reporting
flow) so that head_sha failures are reported by the same warning mechanism used
elsewhere (referencing head_sha, source_repo_root, deps.git().head_sha, doctor
--repair, state.json, refresh).
In `@src/patterns.rs`:
- Around line 38-44: The current matching uses combined.contains(kw) which
treats patterns like "stall.*event loop" as literal text; change the logic to
treat entries in the keyword list as regular expressions: precompile each
pattern string (from the list including "stall.*event loop", "unvalidated.*url",
etc.) into a Regex and use regex.is_match(&combined) instead of
combined.contains(kw); ensure errors from Regex::new are handled (e.g., compile
at init and unwrap or propagate) and keep the original keyword list (no extra
escaping) so wildcard-style patterns work as intended.
- Around line 268-289: Add a regression test in src/patterns.rs that verifies
wildcard (.*) style rules are matched: create a new #[test] function (e.g. fn
classify_wildcard_rule()) that calls classify_pattern with a message that should
trigger an existing wildcard rule and assert the returned Option equals
Some("<expected_rule_id>".into()); reference the existing classify_pattern
usages in patterns.rs to mirror style and naming so the test runs alongside
classify_blocking_in_async and classify_complexity.
In `@src/pipeline.rs`:
- Around line 633-666: This rustfmt sweep inadvertently changed behavior by
altering the InjectionRequest construction and retrieval logic (the block that
builds identifiers, structural_names, text_sample and calls inj.inject,
producing outcome.rendered and context_telemetry); revert this hunk in this PR
so only formatting changes remain and move the modified logic into a separate
commit/PR: restore the previous InjectionRequest payload and retrieval behavior
around InjectionRequest { ... } and the inj.inject(&req) usage (including how
identifiers, structural_names and text are gathered and how
outcome.telemetry/outcome.rendered are used), then create a follow-up change
that introduces the new identifier/import/text collection and any redaction
changes.
In `@src/tools.rs`:
- Around line 39-42: Normalize the registry root when constructing the Tools
struct so resolve_path() and later strip_prefix() use the same canonical basis:
in pub fn new(root: &Path) -> Self replace storing root.to_path_buf() with a
canonicalized root (use std::fs::canonicalize and fall back to
root.to_path_buf() on error) and assign that to self.root; also apply the same
canonicalization change to any other constructor/initialization that sets
self.root (search for other Self { root: ... } initializers) so containment
checks and rendered repo-relative paths remain consistent when the repo is
accessed via symlinks.
---
Outside diff comments:
In `@src/agent.rs`:
- Around line 377-380: The loop over "calls" keeps executing tool calls even if
the investigation budget is exhausted mid-batch; after each successful call via
state.execute_tool_call(&tc, tools, config) (where you push to executed),
immediately check state.limit_reached() (or whether total_bytes_read hit the
cap) and break out of the loop if true to prevent further tool work; apply the
same fix to the other identical loop using execute_tool_call at the later block
(the 405-411 occurrence).
In `@src/calibrator.rs`:
- Around line 1087-1091: The loop handling Verdict::ContextMisleading is
counting duplicate chunk ids from a single feedback entry multiple times; change
it to deduplicate blamed_chunk_ids per entry (e.g., collect into a local
HashSet) and then increment cal.misleading_counts once per unique chunk_id so a
malformed row like ["chunk-a","chunk-a"] only yields one confirmation; ensure
you update the logic around Verdict::ContextMisleading and use
Calibrator::injection_threshold_for as before (the place to modify is the block
that references cal.misleading_counts and blamed_chunk_ids).
- Around line 1073-1079: Calibrator::new currently ignores any non-default
inject_suppress_after and always uses CalibratorConfig::default(); change the
constructor to accept and store the configured suppress budget (e.g., add an
inject_suppress_after parameter or accept a CalibratorConfig) so the field
inject_suppress_after on Calibrator is initialized from the passed configuration
rather than defaults; update the other constructor/creation site referenced
around the second occurrence (symbols: Calibrator::new, CalibratorConfig,
inject_suppress_after, misleading_counts) and ensure the sealing logic that sets
thresholds to f32::INFINITY uses this stored inject_suppress_after to seal after
that many context_misleading confirmations.
In `@src/context/phase6_integration_tests.rs`:
- Around line 115-120: Tests in phase6_integration_tests.rs never exercise the
structural-name exact-match path because every InjectionRequest sets
structural_names to vec![]; add at least one end-to-end test case that supplies
a real structural name so the pipeline→injector contract is exercised. Modify
the InjectionRequest instances (the struct InjectionRequest used in the tests)
to include a structural_names entry matching the symbol under test (e.g., for
file_path "src/auth.rs" and identifier "verify_token" set structural_names:
vec!["auth::verify_token".to_string()] or vec!["verify_token".to_string()]), add
a dedicated case that asserts the expected retrieval/injection behavior, and
repeat this change for the other listed test blocks so the exact-match retrieval
leg runs. Ensure you use the same normalization/format for structural names the
injector expects.
In `@src/domain.rs`:
- Around line 155-172: The ESPHome detection only checks for "yaml" extensions
and thus misses ".yml" files; update both places that use
path.extension().and_then(|e| e.to_str()) == Some("yaml") (the blocks that read
file contents and then check content.starts_with("esphome:") /
contains("\nesphome:")) to accept either "yaml" or "yml" (e.g., compare a
lowercased extension against "yaml" || "yml" or match both strings) so
repositories with *.yml are detected as well.
- Around line 157-173: The PR accidentally changed ESPHome detection logic (the
checks that insert "esphome" into the frameworks set) — revert the behavioral
change or move it out of this formatting PR: restore the original conditional(s)
that inspect file contents for ESPhome (the content.starts_with("esphome:") and
content.contains("\nesphome:") checks used when iterating entries under
project_dir and for root-level YAML files that update the frameworks set), or if
the behavior change is intended, put only that change into a separate functional
PR (and update this PR description) so formatting and logic remain separate.
In `@src/feedback_index.rs`:
- Around line 62-80: build_hybrid (and build) currently calls
LocalEmbedder::new() even when entries.is_empty(), causing unnecessary model
init and returning embedder: Some(_) that find_similar() never uses; update
build_hybrid and build to check entries.is_empty() immediately after loading the
store and either return early with embedder: None (or set embedder = None)
before calling LocalEmbedder::new(), so LocalEmbedder::new() is only invoked
when there are entries to embed; ensure the same guard is applied for the
default build path and references to find_similar remain consistent with
embedder being optional.
In `@src/feedback.rs`:
- Around line 260-267: Replace the src.exists() check with Path::try_exists() so
I/O/stat errors are not collapsed into false; in the Err(e) if e.kind() ==
std::io::ErrorKind::NotFound branch, call src.try_exists(), propagate any Err
returned from try_exists() (return Err(err) rather than treating it as
Ok(false)), and only treat Ok(false) as Ok(false) while treating Ok(true) as
Err(e) to preserve the original race semantics.
🪄 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 Plus
Run ID: d38f4c7b-693f-4058-90d8-c7da366e73d5
📒 Files selected for processing (103)
examples/compare_calibration.rsexamples/compare_retrieval.rssrc/agent.rssrc/analytics.rssrc/cache.rssrc/calibrate.rssrc/calibrator.rssrc/calibrator_trace.rssrc/category.rssrc/cli/mod.rssrc/cli_io.rssrc/config.rssrc/context/bootstrap.rssrc/context/cli.rssrc/context/cli_tests.rssrc/context/config.rssrc/context/config_tests.rssrc/context/extract/astgrep_hcl.rssrc/context/extract/astgrep_hcl_tests.rssrc/context/extract/astgrep_py.rssrc/context/extract/astgrep_py_tests.rssrc/context/extract/astgrep_rust.rssrc/context/extract/astgrep_rust_tests.rssrc/context/extract/astgrep_ts.rssrc/context/extract/astgrep_ts_tests.rssrc/context/extract/dispatch.rssrc/context/extract/dispatch_tests.rssrc/context/extract/markdown.rssrc/context/extract/markdown_tests.rssrc/context/index/builder.rssrc/context/index/builder_tests.rssrc/context/index/state.rssrc/context/index/state_tests.rssrc/context/inject/injector.rssrc/context/inject/plan_tests.rssrc/context/inject/render.rssrc/context/inject/stale_tests.rssrc/context/integration_tests.rssrc/context/phase2_integration_tests.rssrc/context/phase3_integration_tests.rssrc/context/phase4_integration_tests.rssrc/context/phase5_integration_tests.rssrc/context/phase6_integration_tests.rssrc/context/retrieve/bm25_tests.rssrc/context/retrieve/identifiers.rssrc/context/retrieve/identifiers_tests.rssrc/context/retrieve/mod.rssrc/context/retrieve/precedence.rssrc/context/retrieve/precedence_tests.rssrc/context/retrieve/rerank.rssrc/context/retrieve/rerank_tests.rssrc/context/retrieve/retriever.rssrc/context/retrieve/retriever_tests.rssrc/context/retrieve/structural_tests.rssrc/context/retrieve/vector.rssrc/context/retrieve/vector_tests.rssrc/context/store.rssrc/context/types_tests.rssrc/context_enrichment.rssrc/daemon.rssrc/dep_manifest.rssrc/dimensions.rssrc/domain.rssrc/embeddings.rssrc/feedback.rssrc/feedback_index.rssrc/formatting.rssrc/glyphs.rssrc/grounding.rssrc/http_server.rssrc/hydration.rssrc/llm_client.rssrc/main.rssrc/mcp/handler.rssrc/mcp/tools.rssrc/merge.rssrc/metrics.rssrc/output/mod.rssrc/parser.rssrc/patterns.rssrc/pipeline.rssrc/progress.rssrc/redact.rssrc/review.rssrc/review_log.rssrc/stats.rssrc/suppress.rssrc/telemetry.rssrc/test_support.rssrc/threshold_config.rssrc/tools.rssrc/trace_subscriber.rstests/calibrate_backfill.rstests/calibrator_pre_refactor_snapshot.rstests/category_enum_test.rstests/cli.rstests/cli_context_stdout_handling.rstests/cli_feedback_agent.rstests/cli_inbox_drain.rstests/doctor_exit_code.rstests/parallel_review.rstests/review_log.rstests/stats_dimensions.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: analyze (rust)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Do not use emojis in code or output
Support AST-based analysis for Rust files (.rs) including complexity, unsafe code blocks, and unwrap() calls
Use clippy as the linter for Rust files
Include Rust ast-grep rules (5 bundled): block-on-in-async, expect-empty-message, ignored-io-result, silent-error-conversion, string-byte-slice
Files:
src/context/index/state_tests.rssrc/context/integration_tests.rssrc/main.rstests/cli.rssrc/context/retrieve/mod.rssrc/trace_subscriber.rstests/cli_inbox_drain.rstests/stats_dimensions.rstests/doctor_exit_code.rssrc/context/types_tests.rssrc/context/extract/astgrep_hcl_tests.rssrc/context/extract/dispatch_tests.rssrc/context/extract/astgrep_rust.rssrc/category.rstests/calibrate_backfill.rstests/review_log.rssrc/context/retrieve/rerank_tests.rssrc/context/phase5_integration_tests.rssrc/merge.rssrc/context/extract/markdown.rssrc/mcp/tools.rssrc/context/store.rssrc/context/retrieve/identifiers_tests.rssrc/review.rssrc/test_support.rssrc/context/retrieve/bm25_tests.rssrc/context/extract/astgrep_ts.rstests/category_enum_test.rssrc/config.rssrc/context/bootstrap.rssrc/context/retrieve/precedence_tests.rssrc/context/inject/render.rssrc/cli_io.rssrc/context/extract/astgrep_rust_tests.rssrc/context/retrieve/rerank.rstests/cli_context_stdout_handling.rssrc/context/extract/dispatch.rssrc/output/mod.rssrc/context/retrieve/vector_tests.rssrc/context/retrieve/retriever.rssrc/context/index/builder_tests.rssrc/context/retrieve/structural_tests.rssrc/embeddings.rssrc/threshold_config.rssrc/context/extract/astgrep_py_tests.rssrc/context/index/builder.rssrc/context/inject/plan_tests.rssrc/daemon.rssrc/context/retrieve/precedence.rssrc/context/inject/stale_tests.rssrc/context/config.rssrc/context/extract/markdown_tests.rssrc/calibrate.rssrc/context/retrieve/vector.rssrc/http_server.rstests/cli_feedback_agent.rssrc/context/phase3_integration_tests.rssrc/analytics.rssrc/review_log.rssrc/formatting.rsexamples/compare_calibration.rssrc/calibrator_trace.rssrc/dep_manifest.rssrc/cache.rssrc/context/extract/astgrep_ts_tests.rssrc/pipeline.rssrc/context/extract/astgrep_hcl.rssrc/context/extract/astgrep_py.rssrc/dimensions.rssrc/cli/mod.rssrc/glyphs.rssrc/progress.rssrc/domain.rssrc/patterns.rssrc/telemetry.rssrc/llm_client.rstests/parallel_review.rssrc/calibrator.rssrc/grounding.rssrc/parser.rssrc/hydration.rstests/calibrator_pre_refactor_snapshot.rssrc/suppress.rssrc/context/phase2_integration_tests.rssrc/context/phase4_integration_tests.rssrc/context/retrieve/identifiers.rssrc/mcp/handler.rssrc/context/inject/injector.rssrc/context/config_tests.rssrc/feedback_index.rsexamples/compare_retrieval.rssrc/context/index/state.rssrc/redact.rssrc/tools.rssrc/context/cli_tests.rssrc/feedback.rssrc/context/phase6_integration_tests.rssrc/context_enrichment.rssrc/context/retrieve/retriever_tests.rssrc/stats.rssrc/metrics.rssrc/context/cli.rssrc/agent.rs
**/dep_manifest.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/dep_manifest.rs: Insrc/dep_manifest.rs, when pyproject.toml[project].dependencies(PEP 621) is present and well-typed, use it over[tool.poetry.dependencies]
Insrc/dep_manifest.rs, treat pyproject.toml sections that are present but have wrong TOML type (e.g. string instead of array/table) as explicitly empty and do not fall back to requirements.txt; log a tracing::warn
Files:
src/dep_manifest.rs
**/pipeline.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/pipeline.rs: When Context7HttpFetcher::new() fails atrun_reviewstart, setPipelineConfig.context7_disabledto true and skip per-file enrichment via thecontext7_skip_reasonpredicate insrc/pipeline.rs
Build context injector viacontext::bootstrap::build_production_injectorduringrun_reviewto retrieve top-k chunks under token budget with 40% floor, prioritizing symbols
Files:
src/pipeline.rs
**/llm_client.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/llm_client.rs: The base_url validator insrc/llm_client.rs::validate_base_urlmust require HTTPS by default, reject credentials in URL, block private/loopback IPs, and consult QUORUM_ALLOWED_BASE_URL_HOSTS
Insrc/llm_client.rs::validate_base_url, the scheme check must key on whether the resolved host is actually private, not on the env var alone, to prevent QUORUM_ALLOW_PRIVATE_BASE_URL bypass
Files:
src/llm_client.rs
**/calibrator.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/calibrator.rs: When feedback verdict iscontext_misleadingwithblamed_chunks, raise per-chunk injection thresholds viaCalibrator::injection_threshold_for
Permanently seal chunks (set threshold to f32::INFINITY) afterinject_suppress_afterconfirmations of context_misleading verdict
Files:
src/calibrator.rs
**/feedback*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/feedback*.rs: Drop fp_kind when verdicts route through external agent path via--from-agentorfromAgent; log a tracing::warn when fields are dropped
Restrict external-agent verdicts to tp/fp/partial only; reject wontfix and context_misleading at the ingestion chokepoint
Normalize agent names by trimming and lowercasing when recording external verdicts
Clamp external confidence to [0,1] in a NaN-safe manner
Files:
src/feedback_index.rssrc/feedback.rs
**/context_enrichment.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/context_enrichment.rs: Insrc/context_enrichment.rs::enrich_for_review_in_project, parse project dependency manifests (Cargo.toml, package.json, pyproject.toml with requirements.txt fallback) and filter to at most K=5 deps whose names appear in the file's import_targets
Query Context7 insrc/context_enrichment.rswith curated queries viacurated_query_for(name)or language-aware generic queries viageneric_query_for_language(lang)
Files:
src/context_enrichment.rs
**/context*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Cache Context7 resolve results with 24h TTL including negative results, with clock injectable via
CachedContextFetcher::new_with_clockfor testability
Files:
src/context_enrichment.rs
🔇 Additional comments (93)
src/trace_subscriber.rs (1)
9-11: Formatting-only signature reflow looks correct.This change is a pure rustfmt reflow and preserves API shape and behavior.
src/output/mod.rs (2)
129-132:matches!reformat preserves grounding behavior.This is a formatting-only expansion of the existing condition; behavior remains intact and clear.
893-896: Assertion reflow is clean and non-functional.These test changes improve readability without changing semantics, and they keep coverage for grounding-status and ANSI-stripping paths intact.
Also applies to: 907-910, 921-924, 935-938, 949-951, 961-968, 996-999, 1012-1015
src/suppress.rs (4)
140-145: Formatting-only refactor preserves sanitization behavior.Line 140 through Line 145 keep the same control-character stripping and string construction; this change is readability-only and safe.
378-381: Assertion reflow is non-functional and correct.Line 378 through Line 381 are purely formatting changes; test behavior remains identical.
418-418: Builder expression change is style-only.Line 418 keeps identical construction of the test finding; no behavioral impact.
434-438: Multiline vector literal keeps test semantics intact.Line 434 through Line 438 are rustfmt-style reflow with unchanged test data.
src/progress.rs (2)
19-23:detect()implementation is clean and correct
ProgressReporter::detect()is a good, minimal constructor for runtime TTY detection and keeps call sites simple.
26-28: TTY guard refactor is consistent and behavior-preservingThe expanded early-return guards improve readability without changing output behavior in non-TTY contexts.
Also applies to: 34-36, 42-49, 54-62, 67-69, 78-80
src/metrics.rs (1)
84-85: Formatting-only test edits look goodThese are non-behavioral reflows, and the assertions continue to validate the same semantics.
Also applies to: 99-100, 129-130, 140-141, 159-166, 171-172
src/glyphs.rs (2)
38-44: Finite-only scaling and level selection are solid.This keeps NaN/∞ from distorting scale and cleanly centralizes glyph table selection while preserving safe indexing behavior.
Also applies to: 49-53
166-171: Test assertions are stronger and clearer.The updated assertions improve diagnostic quality and add explicit character-range/printability guarantees for both Unicode and ASCII paths.
Also applies to: 183-187, 195-199, 208-213, 223-227
src/redact.rs (6)
102-105: Assertion reflow keeps intent and diagnostics intact.These multiline
assert_eq!rewrites are readability-only and preserve test behavior.Also applies to: 112-115
120-121: Bearer-token fixture update is behavior-neutral.The wrapped literal still exercises JWT-like bearer redaction as intended.
182-183: Private-key fixture wrap is safe.Formatting change does not alter payload semantics or assertion coverage.
246-246: Escaped-quote test matrix expansion is a solid regression guard.Including both
PASSWORDandAPI_KEYescaped-quote cases improves coverage for issue#68.
276-279: Redaction assertion reflow is clear and equivalent.No functional change; failure diagnostics remain useful.
386-390: Case-insensitive auth credential fixtures are well targeted.The added inputs correctly stress token/password/secret/api_key suffix matching paths.
src/dep_manifest.rs (7)
25-37: LGTM!Formatting change only — multi-line closure signature and struct initialization. Logic preserved.
70-98: LGTM!Formatting changes to
langconditional and dependency iteration. Semantics unchanged.
110-115: LGTM!Multi-line reformatting of the empty-check return. No semantic change.
126-313: LGTM!Formatting changes throughout
parse_pyproject. The function correctly:
- Prioritizes PEP 621
[project].dependenciesover Poetry sections.- Treats wrong-type TOML values as explicitly empty with
tracing::warn.- Does not fall back to requirements.txt when a dep section is malformed.
As per coding guidelines: "treat pyproject.toml sections that are present but have wrong TOML type (e.g. string instead of array/table) as explicitly empty and do not fall back to requirements.txt; log a tracing::warn."
315-376: LGTM!Formatting changes to condition layout and struct initializations. PEP 508 handling logic preserved.
378-405: LGTM!Formatting changes to conditional assignment and match arm. Public API signature and fallback logic unchanged.
407-1712: LGTM!Test module reformatted by rustfmt. Comprehensive test coverage for all parsing paths including edge cases (wrong-type handling, PEP 508 direct URLs, Poetry group syntax, deduplication). No semantic changes.
src/patterns.rs (1)
123-164: Formatting + test readability improvements look goodThe signature reflow and assertion formatting keep behavior unchanged, and the added embedding/classification tests improve confidence in intended matching paths.
Also applies to: 171-250
src/domain.rs (1)
584-606: Terraform marker coverage improvement looks goodThe added tests for
terraform.tfvarsandmain.tfclose an important detection gap and improve confidence indetect_domain.src/context/extract/markdown.rs (1)
186-189: Formatting-only match-arm reflow is safe.The H2 heading match condition is unchanged, so behavior remains the same.
src/formatting.rs (1)
97-101: Test assertion reformat is non-functional and correct.Only layout changed; assertion behavior is preserved.
src/config.rs (1)
42-43: Model defaulting line-wrap change is safe.The
QUORUM_MODELlookup and default fallback logic are unchanged.src/context/types_tests.rs (1)
229-232: Assertion reflow preserves test behavior.Condition and failure message are unchanged.
src/context/store.rs (1)
95-97: Error-mapping formatting change is behaviorally neutral.
InvalidDatamapping and line-context message are preserved.tests/doctor_exit_code.rs (1)
24-24: Env chaining rewrite is safe and equivalent.Both required home-related env vars are still configured on the command.
tests/cli.rs (1)
100-104: Write-call reformat is non-functional and correct.Arguments and test flow are unchanged.
tests/cli_inbox_drain.rs (1)
49-49: Both jsonl filter closure rewrites are equivalent.The file-extension predicate logic is unchanged in both locations.
Also applies to: 129-129
src/context/inject/stale_tests.rs (1)
47-47: Formatting-only updates look correct and behavior-preserving.These constructor/fixture reflows keep test semantics unchanged and maintain readability.
Also applies to: 57-57, 69-69, 78-78, 88-88, 99-99, 110-110, 137-137, 176-179, 181-181
src/context/integration_tests.rs (1)
19-23: Formatting change is clean and keeps behavior intact.The multi-line chain remains equivalent and improves readability.
tests/calibrate_backfill.rs (1)
30-34: Assertion reformat is correct and non-functional.No behavior change introduced.
src/context/extract/astgrep_rust.rs (1)
14-14: Formatting updates are safe and maintain existing behavior.Import and initialization reflow are purely stylistic.
Also applies to: 112-112
tests/cli_context_stdout_handling.rs (1)
109-112: Formatting-only assertion change looks good.No semantic impact detected.
src/threshold_config.rs (1)
78-80: Logging macro reformat is correct and behavior-preserving.This is a clean style-only adjustment.
src/context/inject/render.rs (1)
88-89: This formatting change is safe and keeps behavior unchanged.No functional impact on rendering logic.
src/category.rs (1)
77-83: Normalization-chain reformat is correct and non-functional.Mapping semantics are preserved.
tests/category_enum_test.rs (1)
35-42: Formatting update preserves test intent.This hunk is a pure rustfmt-style expansion of existing assertions and does not change behavior.
src/merge.rs (2)
48-50: Comparator reformat is behavior-neutral.The chained
sort_byexpression remains semantically identical after formatting.
225-229: Test assertion reflow is safe.Only layout changed; assertion condition and message are unchanged.
src/context/index/builder.rs (1)
218-219: These builder edits are formatting-only and preserve logic.Prepared statements, JSON serialization, and directory-create error mapping remain functionally the same.
Also applies to: 226-226, 271-272
src/review.rs (2)
108-111: Confidence visitor loop reformat is safe.This is a pure layout change; deserialization fallback semantics are unchanged.
1837-1840: Test assertion formatting is non-functional.The reasoning field assertion is identical in behavior after reflow.
tests/parallel_review.rs (1)
9-13: Parallel review test hunks are formatting-only.Argument order, command behavior, and assertions are preserved; this is rustfmt reflow.
Also applies to: 25-29, 41-45, 59-63, 73-74, 85-89, 96-100
src/cli_io.rs (1)
29-29: Signature compaction is safe.This is a pure formatting change with no functional impact.
src/cache.rs (1)
27-27: Cache changes are formatting-only and behavior-preserving.Lru initialization,
get_or_parsesignature layout,cache.putstructure, and test chaining remain semantically identical.Also applies to: 40-40, 55-61, 180-182
src/context/config.rs (1)
482-484: Context config assignment reflow is non-functional.The
max_source_size_mbfallback logic is unchanged; this is formatting-only.src/context/retrieve/rerank.rs (1)
92-102: Formatting-only edits are behaviorally safe.The recency and path boost changes preserve existing scoring behavior.
Also applies to: 133-137
src/context/extract/markdown_tests.rs (1)
22-29: Test updates look correct and stable.These edits are formatting-only plus a valid chunk-kind assertion improvement.
Also applies to: 135-142, 166-169, 175-182
src/mcp/tools.rs (1)
2-2: No functional impact in these MCP tool changes.The modified lines are formatting/import-order adjustments in production/tests.
Also applies to: 213-216, 352-353
src/context/retrieve/vector.rs (1)
97-99: Placeholder construction refactor is safe.The new multiline form preserves the same SQL placeholder output.
src/context/extract/dispatch.rs (1)
18-18: Dispatch formatting changes preserve behavior.No functional drift in extractor routing from these edits.
Also applies to: 258-269
src/context/extract/dispatch_tests.rs (1)
6-6: Import reorder in tests is fine.No correctness or reliability impact.
src/context/retrieve/precedence.rs (1)
101-101: Precedence formatting edits are behavior-neutral.The ordering logic and comparator semantics are preserved.
Also applies to: 149-149, 158-163
src/context/retrieve/mod.rs (1)
32-32: Re-export reorder is safe.Publicly exported symbols are unchanged.
src/context/retrieve/retriever.rs (1)
13-13: Formatting-only hunks look correct.No functional concerns in these touched segments; behavior and contracts remain intact.
Also applies to: 92-95
src/context/index/state.rs (1)
104-104: Touched lines are safe and behavior-preserving.No blocking issues found in these changes.
Also applies to: 117-122, 189-189
src/context/index/state_tests.rs (1)
5-5: Import reordering is clean.No functional or test-contract impact from this change.
src/cli/mod.rs (1)
107-117: CLI touched hunks look good.Validation behavior and test intent are preserved in these edits.
Also applies to: 630-630, 695-1522
src/context/retrieve/identifiers.rs (1)
69-70: Identifier retrieval edits are solid.No blocking correctness, reliability, or security concerns in the touched lines.
Also applies to: 160-160, 199-220
examples/compare_calibration.rs (1)
20-27: Example updates look consistent and safe.No blocking issues found in the changed sections.
Also applies to: 33-40, 46-50, 69-73, 79-85, 121-277
src/dimensions.rs (1)
53-57: Dimension aggregation changes are in good shape.No blocking issues identified in these touched segments.
Also applies to: 63-74, 156-160, 180-198, 240-245, 276-279, 302-304, 320-350, 384-391, 405-420, 450-970
src/context/retrieve/structural_tests.rs (1)
97-97: Structural retrieval test hunks look correct.No concerns in the modified test segments.
Also applies to: 109-114, 176-176, 178-178, 207-207
src/embeddings.rs (1)
7-7: Formatting-only changes preserve behaviorThe updated import ordering, iterator-chain reflow, early-return formatting, and test assert reflow are semantically unchanged and look good.
Also applies to: 26-29, 38-40, 44-46, 89-99
src/context/config_tests.rs (1)
27-30: Test assertions are clearer and still correctThe variant checks and multi-line error assertions improve failure readability without changing test intent.
Also applies to: 40-43, 196-199, 260-263, 293-296
src/daemon.rs (1)
60-60: Daemon/event-loop changes are formatting-onlyThese edits are purely presentational and keep runtime behavior unchanged.
Also applies to: 106-107, 134-136, 140-142, 146-148
src/context/extract/astgrep_rust_tests.rs (1)
23-34: Test refinements are soundThe assertion reflow and added neighboring-symbol checks improve clarity/coverage without introducing risk.
Also applies to: 53-57, 172-183
src/context/index/builder_tests.rs (1)
11-16: Builder test updates look correctThese are formatting/readability changes only, with unchanged query/assert semantics.
Also applies to: 103-103, 360-362, 401-408
src/grounding.rs (1)
11-59: Grounding changes are semantically stableThe reflowed constants, predicates, and test assertions preserve behavior and improve readability.
Also applies to: 85-88, 123-131, 305-308, 313-315, 417-422, 565-568, 698-701
src/calibrate.rs (1)
293-297: Calibration module changes look cleanReviewed changed logic and test hunks: edits are predominantly formatting/assert readability, with preserved behavior in the touched functional paths.
Also applies to: 568-571, 686-690, 782-785, 800-803, 850-854, 862-866, 899-903, 921-923, 931-935, 954-973, 979-992, 1003-1005, 1008-1010, 1022-1024, 1037-1039, 1094-1100, 1113-1117, 1129-1132, 1137-1144, 1167-1172, 1197-1204, 1206-1209, 1377-1381, 1384-1387, 1395-1400, 1409-1411, 1414-1417, 1425-1430, 1445-1450, 1463-1467, 1480-1489, 1501-1506, 1516-1535, 1541-1542, 1556-1575, 1581-1588, 1598-1615, 1621-1622, 1630-1638, 1643-1644, 1651-1659, 1664-1665, 1678-1708, 1714-1720, 1731-1749, 1755-1761, 1779-1783, 1792-1803, 1825-1829, 1857-1865, 1882-1890, 1899-1909
src/telemetry.rs (1)
4-6: Telemetry updates are well-formed and backward-compatibleThe added context7 fields are safely defaulted for legacy rows, and the parser/test edits maintain existing behavior.
Also applies to: 18-24, 149-153, 176-176, 189-193, 307-309, 356-359, 625-626, 633-634
src/context/extract/astgrep_ts.rs (1)
15-15: Formatting-only edits look correct.These changes are style-only and preserve behavior.
Also applies to: 112-112
src/context/inject/plan_tests.rs (1)
1-1: Test formatting updates are safe.No logic or coverage regression introduced by these edits.
Also applies to: 125-125
src/review_log.rs (1)
295-297: Changed hunks are consistent formatting-only updates.No functional delta detected in these reflows.
Also applies to: 453-457, 557-565, 581-587, 596-600, 625-625, 643-647, 702-705, 822-825, 836-843, 919-922
tests/review_log.rs (1)
37-40: Integration test reformat is clean.Assertions and ID extraction logic remain equivalent.
Also applies to: 82-89
src/context/extract/astgrep_py.rs (1)
20-20: Formatting changes are safe and consistent.No correctness or extraction-behavior concerns in these modified lines.
Also applies to: 32-32, 124-124, 425-425, 453-453, 503-503
src/context/phase2_integration_tests.rs (1)
9-9: Test-only formatting updates look good.No behavior change introduced.
Also applies to: 41-45, 47-47, 104-118
src/context/extract/astgrep_hcl_tests.rs (1)
66-69: HCL extractor test reflow is correct.No functional impact from these edits.
Also applies to: 155-156
src/context/extract/astgrep_hcl.rs (1)
20-20: Formatting-only hunks are clean.These edits preserve runtime behavior.
Also applies to: 128-128, 149-152, 277-277
src/context/bootstrap.rs (1)
155-177: Formatting-only reflow looks correct.The retriever closure still preserves the same capture and query-filtering behavior, and the test import reorder is inert.
Also applies to: 240-240
src/test_support.rs (1)
5-6: These test-double edits are safe.Import ordering and multiline formatting do not change the FakeReviewer or FakeAgentReviewer behavior here.
Also applies to: 41-64, 75-77
src/context/phase4_integration_tests.rs (1)
10-10: Test updates are consistent with the current RetrievalQuery shape.The added
structural_names: vec![]initializers and formatting-only reflow keep these integration tests aligned without changing intent.Also applies to: 30-32, 40-43, 54-54, 67-67, 92-92, 117-117, 141-141
src/context/phase3_integration_tests.rs (1)
10-10: No concerns with these formatting-only test edits.The reordered import and wrapped expressions preserve the existing phase 3 test behavior.
Also applies to: 48-48, 67-67, 94-96
tests/stats_dimensions.rs (1)
67-100: The strengthened assertions look good.These hunks improve failure diagnostics and make the expected stats output shape more explicit without introducing behavioral risk.
Also applies to: 123-137, 155-168
src/context/extract/astgrep_ts_tests.rs (1)
25-36: These TS extractor test tightenings are useful and safe.The new assertions add good regression coverage around signature preservation and body stripping, and the fixture read reflow is behaviorally neutral.
Also applies to: 53-59, 235-246, 251-251
src/context/phase5_integration_tests.rs (1)
9-17: These phase 5 test edits look fine.The import reflow is inert, and the added
verify_tokenassertion improves the end-to-end signal without changing the test setup.Also applies to: 96-113
src/context/extract/astgrep_py_tests.rs (1)
22-22: These Python extractor test changes are safe.They tighten the assertions slightly and reflow formatting, but the covered behavior and expectations stay the same.
Also applies to: 40-45, 149-152, 287-291
| eprintln!( | ||
| "Context7 query_docs: HTTP {} - {}", | ||
| status, | ||
| &body[..200.min(body.len())] | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and examine the code around lines 613-617
cat -n src/context_enrichment.rs | sed -n '600,630p'Repository: jsnyder/quorum
Length of output: 1189
🏁 Script executed:
# Also check the broader context to understand the variable types
cat -n src/context_enrichment.rs | sed -n '580,640p'Repository: jsnyder/quorum
Length of output: 2613
Avoid byte-slicing the HTTP error body.
Line 616 can panic on a non-ASCII response body because Rust String slices must end on UTF-8 character boundaries. That makes a recoverable Context7 4xx/5xx fail on the logging path instead.
Suggested fix
- eprintln!(
- "Context7 query_docs: HTTP {} - {}",
- status,
- &body[..200.min(body.len())]
- );
+ let preview: String = body.chars().take(200).collect();
+ eprintln!("Context7 query_docs: HTTP {} - {}", status, preview);📝 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.
| eprintln!( | |
| "Context7 query_docs: HTTP {} - {}", | |
| status, | |
| &body[..200.min(body.len())] | |
| ); | |
| let preview: String = body.chars().take(200).collect(); | |
| eprintln!("Context7 query_docs: HTTP {} - {}", status, preview); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/context_enrichment.rs` around lines 613 - 617, The log call that prints
"Context7 query_docs" slices the HTTP response body by bytes and can panic on
non-UTF-8/partial-UTF-8 boundaries; update the logging in the Context7/
query_docs path (the eprintln! with "Context7 query_docs: HTTP {} - {}") to
produce a safe truncated string instead of &body[..200.min(body.len())] — for
example use a UTF-8-safe truncation like body.get(..200).unwrap_or(&body) or
body.chars().take(200).collect::<String>() so the logger never panics on
non-ASCII or multi-byte characters.
There was a problem hiding this comment.
Pre-existing code, not introduced by this formatting PR. Will triage separately.
There was a problem hiding this comment.
@jsnyder, understood — the byte-slicing logic predates this PR and the diff only reformatted the eprintln!. Fair point.
Would you like me to open a GitHub issue to track the UTF-8–safe truncation fix so it doesn't get lost?
🧠 Learnings used
Learnt from: jsnyder
Repo: jsnyder/quorum PR: 235
File: src/dimensions.rs:408-408
Timestamp: 2026-05-06T02:37:09.757Z
Learning: Do not flag indentation-only differences in Rust struct initializers as syntax errors. Indentation variations are cosmetic and handled by rustfmt. Only report real syntax/semantic issues (e.g., missing commas between fields, incorrect field names/types, or actual compile errors). Apply this guidance to all Rust source files in the repo.
| let head_sha = match source_repo_root(entry) { | ||
| Some(root) if root.exists() => { | ||
| deps.git().head_sha(root).unwrap_or(None) | ||
| } | ||
| Some(root) if root.exists() => deps.git().head_sha(root).unwrap_or(None), | ||
| _ => None, |
There was a problem hiding this comment.
Do not hide head_sha failures during repair.
Turning deps.git().head_sha(root) errors into None makes doctor --repair look successful while writing a state.json that can never match HEAD, so later refresh calls keep rebuilding unnecessarily. Let this bubble into the existing best-effort warning path instead.
Suggested change
- Some(root) if root.exists() => deps.git().head_sha(root).unwrap_or(None),
+ Some(root) if root.exists() => deps
+ .git()
+ .head_sha(root)
+ .map_err(|e| anyhow!("git head_sha({}): {e}", root.display()))?,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/context/cli.rs` around lines 1950 - 1952, The code is swallowing errors
from deps.git().head_sha(root) by using unwrap_or(None), causing doctor --repair
to appear successful while writing an invalid state.json; change the head_sha
extraction in the source_repo_root match so that errors from deps.git().head_sha
propagate into the existing best-effort warning path instead of being converted
to None: replace the unwrap_or(None) use on deps.git().head_sha(root) with logic
that returns or forwards the Err (or maps it into the same warning-reporting
flow) so that head_sha failures are reported by the same warning mechanism used
elsewhere (referencing head_sha, source_repo_root, deps.git().head_sha, doctor
--repair, state.json, refresh).
There was a problem hiding this comment.
Pre-existing code, not introduced by this formatting PR. Will triage separately.
There was a problem hiding this comment.
@jsnyder, understood — the unwrap_or(None) behavior predates this formatting sweep and is out of scope here. Would you like me to open a follow-up GitHub issue to track the fix so it doesn't get lost?
🧠 Learnings used
Learnt from: jsnyder
Repo: jsnyder/quorum PR: 235
File: src/dimensions.rs:408-408
Timestamp: 2026-05-06T02:37:09.757Z
Learning: Do not flag indentation-only differences in Rust struct initializers as syntax errors. Indentation variations are cosmetic and handled by rustfmt. Only report real syntax/semantic issues (e.g., missing commas between fields, incorrect field names/types, or actual compile errors). Apply this guidance to all Rust source files in the repo.
| fn classify_blocking_in_async() { | ||
| assert_eq!(classify_pattern("Blocking future.result() in async", "", "concurrency"), Some("blocking_in_async".into())); | ||
| assert_eq!(classify_pattern("block_in_place can panic", "", "bug"), Some("blocking_in_async".into())); | ||
| assert_eq!( | ||
| classify_pattern("Blocking future.result() in async", "", "concurrency"), | ||
| Some("blocking_in_async".into()) | ||
| ); | ||
| assert_eq!( | ||
| classify_pattern("block_in_place can panic", "", "bug"), | ||
| Some("blocking_in_async".into()) | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn classify_complexity() { | ||
| assert_eq!(classify_pattern("Function foo has cyclomatic complexity 12", "", "complexity"), Some("complexity_high".into())); | ||
| assert_eq!( | ||
| classify_pattern( | ||
| "Function foo has cyclomatic complexity 12", | ||
| "", | ||
| "complexity" | ||
| ), | ||
| Some("complexity_high".into()) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a regression test for wildcard-rule matching
Current tests validate direct keyword matches but not the .* style rules. Add one focused case so wildcard-style behavior stays protected.
Suggested test addition
#[test]
fn classify_blocking_in_async() {
assert_eq!(
classify_pattern("Blocking future.result() in async", "", "concurrency"),
Some("blocking_in_async".into())
);
assert_eq!(
classify_pattern("block_in_place can panic", "", "bug"),
Some("blocking_in_async".into())
);
+ assert_eq!(
+ classify_pattern("This can stall the reactor thread", "", "concurrency"),
+ Some("blocking_in_async".into())
+ );
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/patterns.rs` around lines 268 - 289, Add a regression test in
src/patterns.rs that verifies wildcard (.*) style rules are matched: create a
new #[test] function (e.g. fn classify_wildcard_rule()) that calls
classify_pattern with a message that should trigger an existing wildcard rule
and assert the returned Option equals Some("<expected_rule_id>".into());
reference the existing classify_pattern usages in patterns.rs to mirror style
and naming so the test runs alongside classify_blocking_in_async and
classify_complexity.
There was a problem hiding this comment.
Pre-existing code, not introduced by this formatting PR. Will triage separately.
There was a problem hiding this comment.
@jsnyder Understood — the wildcard rule matching is pre-existing and out of scope for this formatting sweep. Would you like me to open a GitHub issue to track adding the regression test for .*-style rules in a follow-up PR?
🧠 Learnings used
Learnt from: jsnyder
Repo: jsnyder/quorum PR: 235
File: src/dimensions.rs:408-408
Timestamp: 2026-05-06T02:37:09.757Z
Learning: Do not flag indentation-only differences in Rust struct initializers as syntax errors. Indentation variations are cosmetic and handled by rustfmt. Only report real syntax/semantic issues (e.g., missing commas between fields, incorrect field names/types, or actual compile errors). Apply this guidance to all Rust source files in the repo.
| pub fn new(root: &Path) -> Self { | ||
| Self { root: root.to_path_buf() } | ||
| Self { | ||
| root: root.to_path_buf(), | ||
| } |
There was a problem hiding this comment.
Normalize the registry root once so containment checks and rendered paths use the same basis.
Right now resolve_path() compares against a canonical root, but self.root itself stays non-canonical and is later reused by strip_prefix(). If the repo root is accessed through a symlink, subdirectory traversals can fall back to absolute canonical paths in tool output instead of stable repo-relative paths.
Suggested fix
pub struct ToolRegistry {
root: PathBuf,
}
impl ToolRegistry {
pub fn new(root: &Path) -> Self {
- Self {
- root: root.to_path_buf(),
- }
+ Self {
+ root: root.canonicalize().unwrap_or_else(|_| root.to_path_buf()),
+ }
}Also applies to: 111-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tools.rs` around lines 39 - 42, Normalize the registry root when
constructing the Tools struct so resolve_path() and later strip_prefix() use the
same canonical basis: in pub fn new(root: &Path) -> Self replace storing
root.to_path_buf() with a canonicalized root (use std::fs::canonicalize and fall
back to root.to_path_buf() on error) and assign that to self.root; also apply
the same canonicalization change to any other constructor/initialization that
sets self.root (search for other Self { root: ... } initializers) so containment
checks and rendered repo-relative paths remain consistent when the repo is
accessed via symlinks.
There was a problem hiding this comment.
Pre-existing code, not introduced by this formatting PR. Will triage separately.
There was a problem hiding this comment.
@jsnyder Understood — the concern is pre-existing and out of scope for this formatting sweep. Would you like me to open a GitHub issue to track the ToolRegistry::new root canonicalization fix so it doesn't get lost?
🧠 Learnings used
Learnt from: jsnyder
Repo: jsnyder/quorum PR: 235
File: src/dimensions.rs:408-408
Timestamp: 2026-05-06T02:37:09.757Z
Learning: Do not flag indentation-only differences in Rust struct initializers as syntax errors. Indentation variations are cosmetic and handled by rustfmt. Only report real syntax/semantic issues (e.g., missing commas between fields, incorrect field names/types, or actual compile errors). Apply this guidance to all Rust source files in the repo.
Closes #216 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
src/domain.rs (1)
402-403:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove the duplicated
#[test]attribute to unblock CI.Lines 402–403 contain duplicate
#[test]annotations on the same function, triggering theduplicate-macro-attributesclippy error and blocking the pipeline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain.rs` around lines 402 - 403, Remove the duplicated #[test] attribute above the test function in src/domain.rs so only a single #[test] annotation remains; locate the consecutive duplicate #[test] lines (the pair shown in the diff) and delete one of them to satisfy the duplicate-macro-attributes lint and restore CI.tests/parallel_review.rs (1)
59-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert command status in
parallel_json_output_validbefore validating stdout JSON.Line 59 captures output, but the test never checks whether the command exited successfully (or with expected non-critical code). A hard failure with empty stdout can still pass this test.
Proposed fix
let output = Command::cargo_bin("quorum") .unwrap() .arg("review") .arg("--parallel") .arg("2") .arg("--json") .arg(dir.path().join("file1.py")) .arg(dir.path().join("file2.py")) .arg(dir.path().join("file3.py")) .output() .unwrap(); + let code = output.status.code().unwrap_or(2); + assert!( + code <= 1, + "expected exit code 0 or 1, got {}. stderr: {}", + code, + String::from_utf8_lossy(&output.stderr) + ); let stdout = String::from_utf8_lossy(&output.stdout);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/parallel_review.rs` around lines 59 - 70, The test parallel_json_output_valid captures the command output into variable output but never asserts the process exit status before parsing stdout; update the test (the Command::cargo_bin("quorum")... .output() result handling) to assert the command exited successfully (e.g., check output.status.success() or compare output.status.code() to the expected code) immediately after .unwrap() and before validating/decoding output.stdout to ensure the test fails on process errors rather than empty or invalid JSON.src/cache.rs (1)
40-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCache key must include language to avoid cross-language parse collisions.
At Line 41, the key is built from content only. If identical text is parsed as two different
Languagevalues, the second call can return the first language’stree, which is incorrect.Proposed fix
- let hash = Self::content_hash(content); + let hash = format!("{lang:?}:{}", Self::content_hash(content));Also add a regression test using identical content with two different languages to confirm two cache entries are created.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cache.rs` around lines 40 - 45, The cache key in get_or_parse currently uses only content_hash(content) which allows collisions across different Language values; change the key to incorporate the Language (e.g., combine content_hash(content) with lang.id() or a language-specific discriminator) so cache lookups and inserts in self.inner use this combined key (update where cache.get(&hash) and cache.insert(...) are used), and ensure hits bookkeeping (self.hits) remains correct; additionally add a regression test that calls get_or_parse with identical content but two different Language values and asserts that two separate cache entries are created (or that the second parse returns a tree for the correct language).src/hydration.rs (5)
107-121:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCaller blast radius misses multiline signature edits.
This only checks whether the function's start line falls inside the changed range. A signature edit on a later declaration line, e.g. a multiline
fn foo(\n arg: T,\n), will be treated like a body-only change andctx.callerswill stay empty. You need to track the declaration/signature span separately and test overlap against that span, not justfstart.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hydration.rs` around lines 107 - 121, The caller-blast logic only checks fstart against changed_lines so multiline signatures (e.g. fn foo(\n arg: T,) whose signature extends past fstart) are missed; update the function metadata and the overlap test to use the function's signature/declaration span instead of just fstart: extend the tuple stored in all_funcs to include signature_start and signature_end (or a decl_span), then in the loop compare the changed range against that signature/declaration span (checking any overlap) and call find_callers_of when the signature span intersects the changed region; ensure you still handle the original body-span case if needed and update references to fstart/_fend (e.g., where all_funcs is constructed and where callers are computed) to use the new signature span variables so ctx.callers is populated for multiline signature edits.
608-616:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
strip_prefix("+++ b/")to replace the manual slice.The pattern of checking
starts_with("+++ b/")then usingline[6..]is flagged by clippy'smanual_striplint. Since the prefix check already precedes this line, safely use:current_file = Some(line.strip_prefix("+++ b/").unwrap().to_string());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hydration.rs` around lines 608 - 616, Replace the manual slice logic in the loop that iterates over diff.lines(): instead of checking starts_with("+++ b/") and then doing line[6..].to_string(), call line.strip_prefix("+++ b/").unwrap().to_string() to get the filename and assign it to current_file; this change should be applied where current_file = Some(...) is set within the for line in diff.lines() loop that also references current_ranges and results.
195-202:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCollapse the nested
ifusing if-let chains to resolve the clippycollapsible_iflint.The code can be rewritten as:
if func_kinds.contains(&kind) && let Some(name_node) = node.child_by_field_name("name") { ... }. Note that a similar pattern appears at lines 203-208 (type_kinds) and should be fixed the same way.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hydration.rs` around lines 195 - 202, The nested ifs should be collapsed into an if-let chain to satisfy clippy's collapsible_if lint: replace the pattern that checks func_kinds.contains(&kind) and then does if let Some(name_node) = node.child_by_field_name("name") with a single combined condition (e.g., if func_kinds.contains(&kind) && let Some(name_node) = ... ) so you still obtain name_node and use source, node, sig, and push to funcs; apply the same refactor for the type_kinds branch (the code that currently checks type_kinds.contains(&kind) and then if let Some(name_node) = ...) so both places eliminate the nested if and keep existing behavior.
317-381:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrip
typemodifier when extracting local binding names from TypeScript type-only imports.The parser fails on type-only imports because it doesn't account for the
typekeyword. Cases likeimport type Foo from "x"andimport { type Foo } from "x"produce"type Foo"and"type"instead ofFoo. This causescollect_import_refs_in_rangeto skip these imports when matching against identifiers in the changed range.Strip the leading
typeprefix in both the default-import branch (before line 353, when pushing non-aliased head parts) and the named-import branch (before line 370, when splitting the inner clause).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hydration.rs` around lines 317 - 381, The import-name extractor (working on variables clause/head/group and pushing into names) doesn't strip the TypeScript `type ` modifier, so `import type Foo` and `import { type Foo }` yield incorrect names; update the logic in the default-import branch (the loop over head.split(',') where you push part.to_string()) to first strip a leading "type " (e.g., part = part.strip_prefix("type ").unwrap_or(part).trim()), and similarly in the named-import branch (where you compute name from part or after_as) strip a leading "type " before checking emptiness or "*" and before pushing into names; ensure both branches trim after stripping so `type` and `type Foo` are handled correctly.
181-190:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRefactor these helper functions to use context structs for parameters.
collect_definitions(line 181),collect_calls_in_range(line 403), andcollect_type_refs_in_range(line 466) each exceed clippy's argument limit with 9-10 parameters. Instead of using#[allow(clippy::too_many_arguments)], bundle the traversal inputs/outputs into small context structs to keep these signatures clean.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hydration.rs` around lines 181 - 190, collect_definitions, collect_calls_in_range, and collect_type_refs_in_range currently take 9–10 parameters; refactor by creating small context structs (e.g., DefinitionContext, CallTraversalContext, TypeRefContext) that bundle related input fields (node, source, func_kinds/type_kinds/import_kinds_list, range info, etc.) and output collectors (e.g., funcs, types, imports or a Vec field) and change each function signature to accept the appropriate context struct(s) instead of many arguments; update call sites to construct and pass these contexts, move any mutable collectors into the context so they’re updated in-place, and remove the #[allow(clippy::too_many_arguments)] annotations afterward.src/agent.rs (1)
317-336: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winPlease keep the rustfmt sweep behavior-free.
This hunk adds new prompt/message semantics: assistant tool-call serialization,
<tool_output>wrapping/escaping, and different forced-final-turn handling. Those are real behavior changes, not formatting, so they should move to a dedicated follow-up PR or commit to preserve the low-risk fmt sweep objective.Also applies to: 395-423
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent.rs` around lines 317 - 336, This change introduces behavioral logic (assistant tool-call serialization and new message semantics) in append_assistant_tool_calls and the other hunk (around lines where assistant tool-call/`<tool_output>` wrapping and forced-final-turn handling were added); revert those behavioral changes so this PR only contains rustfmt/formatting edits — remove the new JSON serialization block and the push of "tool_calls" from append_assistant_tool_calls (restore the previous formatting-only implementation), and similarly undo the behavioral edits in the other affected function(s) so that only whitespace/formatting remains in this commit; move the new semantics (tool-call serialization, `<tool_output>` escaping, forced-final-turn logic) into a separate follow-up commit/PR referencing the functions append_assistant_tool_calls and the other modified functions.src/calibrator.rs (2)
1052-1129:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSplit the new calibrator behavior out of the rustfmt sweep.
This block adds a new public API and new runtime behavior in a PR whose stated goal is a formatting-only sweep. Keeping functional changes here defeats the one-shot blame-ignore formatting commit and makes regressions much harder to isolate or revert.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/calibrator.rs` around lines 1052 - 1129, The new Calibrator type and its methods (Calibrator, Calibrator::new, Calibrator::from_feedback, Calibrator::with_suppress_after, Calibrator::record_misleading, Calibrator::injection_threshold_for and the misleading_counts/inject_suppress_after fields) introduce runtime behavior in what should be a formatting-only sweep; revert or move these functional additions out of this commit by removing the Calibrator struct and its impl from this diff (or stash them to a separate feature branch/commit) so this PR only contains rustfmt/whitespace changes, then re-run rustfmt and update the PR to include only formatting edits.
1073-1080:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject invalid injection floors at construction.
injection_threshold_for()assumes a finite floor in[0.0, 1.0], butnew()currently accepts anyf32. Ifinject_floor > 1.0, each confirmation makes the threshold go down instead of up;NaNwill also poison every later comparison.Proposed fix
pub fn new(inject_floor: f32) -> Self { + let inject_floor = if inject_floor.is_finite() { + inject_floor.clamp(0.0, 1.0) + } else { + 0.0 + }; let defaults = CalibratorConfig::default(); Self { inject_floor, inject_suppress_after: defaults.inject_suppress_after, misleading_counts: HashMap::new(),Also applies to: 1117-1127
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/calibrator.rs` around lines 1073 - 1080, The constructor Calibrator::new currently accepts any f32 for inject_floor; add a validation check that inject_floor.is_finite() and 0.0 <= inject_floor <= 1.0 and reject otherwise (panic with a clear message or return a Result per project convention). Update the new(inject_floor: f32) implementation to perform this check before constructing the struct (the struct fields include inject_floor, inject_suppress_after, misleading_counts) and apply the same validation to the other constructor/initializer that accepts inject_floor (the one around lines 1117-1127 mentioned in the comment) so NaN or >1.0 values cannot be stored.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/calibrator.rs`:
- Around line 2201-2215: The calls to calibrate (e.g., the call computing
result1 and result2 and the other mentions around lines 2259-2268 and 2295) are
passing &vec![...] which triggers the useless_vec clippy lint; change those
borrowed Vec constructions to slice literals by passing &[human_fp.clone(),
auto_fp.clone()] (or &[auto_fp.clone(), auto_fp]) instead of &vec![...],
ensuring the argument type still matches calibrate(feedback: &[FeedbackEntry])
and preserving clones where needed.
- Around line 3029-3032: Replace the use of Vec::get(0) with Vec::first() on the
findings vector to satisfy the clippy get_first lint: update the eprintln call
that currently uses result.findings.get(0) to use result.findings.first()
(preserving the map/Option handling and cloning of severity) so the printed
behavior remains identical while removing the lint warning in the eprintln!
invocation that references result.findings.
In `@src/context/config_tests.rs`:
- Around line 27-30: The assertions are moving the non-Copy SourceLocation out
of a borrowed vector element; change the matches! patterns to borrow the field
(e.g. use &config.sources[0].location and match against &SourceLocation::Git {
.. }) so you don't move the value; apply the same change for the other
occurrence that tests SourceLocation (around the second assertion block at lines
40-43).
In `@src/feedback.rs`:
- Around line 1371-1375: The assertion on inner.get("confidence") uses
map_or(true, |c| c.is_null()), which Clippy flags; replace that check with the
idiomatic is_none_or(|c| c.is_null()) on inner.get("confidence") in the assert!
so it reads that the "confidence" entry must be none or null while keeping the
existing error message and location (the assert! around
inner.get("confidence")).
In `@src/progress.rs`:
- Around line 19-23: Remove the new public API method ProgressReporter::detect()
from this formatting-only change: delete the detect() function
declaration/implementation in src/progress.rs (the self.is_tty =
io::stderr().is_terminal() logic) and revert any call sites introduced in this
PR so the behavior remains unchanged; instead, extract that method and its
tests/uses into a separate focused PR for the functional addition and optionally
leave a brief comment referencing the new PR for future work.
---
Outside diff comments:
In `@src/agent.rs`:
- Around line 317-336: This change introduces behavioral logic (assistant
tool-call serialization and new message semantics) in
append_assistant_tool_calls and the other hunk (around lines where assistant
tool-call/`<tool_output>` wrapping and forced-final-turn handling were added);
revert those behavioral changes so this PR only contains rustfmt/formatting
edits — remove the new JSON serialization block and the push of "tool_calls"
from append_assistant_tool_calls (restore the previous formatting-only
implementation), and similarly undo the behavioral edits in the other affected
function(s) so that only whitespace/formatting remains in this commit; move the
new semantics (tool-call serialization, `<tool_output>` escaping,
forced-final-turn logic) into a separate follow-up commit/PR referencing the
functions append_assistant_tool_calls and the other modified functions.
In `@src/cache.rs`:
- Around line 40-45: The cache key in get_or_parse currently uses only
content_hash(content) which allows collisions across different Language values;
change the key to incorporate the Language (e.g., combine content_hash(content)
with lang.id() or a language-specific discriminator) so cache lookups and
inserts in self.inner use this combined key (update where cache.get(&hash) and
cache.insert(...) are used), and ensure hits bookkeeping (self.hits) remains
correct; additionally add a regression test that calls get_or_parse with
identical content but two different Language values and asserts that two
separate cache entries are created (or that the second parse returns a tree for
the correct language).
In `@src/calibrator.rs`:
- Around line 1052-1129: The new Calibrator type and its methods (Calibrator,
Calibrator::new, Calibrator::from_feedback, Calibrator::with_suppress_after,
Calibrator::record_misleading, Calibrator::injection_threshold_for and the
misleading_counts/inject_suppress_after fields) introduce runtime behavior in
what should be a formatting-only sweep; revert or move these functional
additions out of this commit by removing the Calibrator struct and its impl from
this diff (or stash them to a separate feature branch/commit) so this PR only
contains rustfmt/whitespace changes, then re-run rustfmt and update the PR to
include only formatting edits.
- Around line 1073-1080: The constructor Calibrator::new currently accepts any
f32 for inject_floor; add a validation check that inject_floor.is_finite() and
0.0 <= inject_floor <= 1.0 and reject otherwise (panic with a clear message or
return a Result per project convention). Update the new(inject_floor: f32)
implementation to perform this check before constructing the struct (the struct
fields include inject_floor, inject_suppress_after, misleading_counts) and apply
the same validation to the other constructor/initializer that accepts
inject_floor (the one around lines 1117-1127 mentioned in the comment) so NaN or
>1.0 values cannot be stored.
In `@src/domain.rs`:
- Around line 402-403: Remove the duplicated #[test] attribute above the test
function in src/domain.rs so only a single #[test] annotation remains; locate
the consecutive duplicate #[test] lines (the pair shown in the diff) and delete
one of them to satisfy the duplicate-macro-attributes lint and restore CI.
In `@src/hydration.rs`:
- Around line 107-121: The caller-blast logic only checks fstart against
changed_lines so multiline signatures (e.g. fn foo(\n arg: T,) whose signature
extends past fstart) are missed; update the function metadata and the overlap
test to use the function's signature/declaration span instead of just fstart:
extend the tuple stored in all_funcs to include signature_start and
signature_end (or a decl_span), then in the loop compare the changed range
against that signature/declaration span (checking any overlap) and call
find_callers_of when the signature span intersects the changed region; ensure
you still handle the original body-span case if needed and update references to
fstart/_fend (e.g., where all_funcs is constructed and where callers are
computed) to use the new signature span variables so ctx.callers is populated
for multiline signature edits.
- Around line 608-616: Replace the manual slice logic in the loop that iterates
over diff.lines(): instead of checking starts_with("+++ b/") and then doing
line[6..].to_string(), call line.strip_prefix("+++ b/").unwrap().to_string() to
get the filename and assign it to current_file; this change should be applied
where current_file = Some(...) is set within the for line in diff.lines() loop
that also references current_ranges and results.
- Around line 195-202: The nested ifs should be collapsed into an if-let chain
to satisfy clippy's collapsible_if lint: replace the pattern that checks
func_kinds.contains(&kind) and then does if let Some(name_node) =
node.child_by_field_name("name") with a single combined condition (e.g., if
func_kinds.contains(&kind) && let Some(name_node) = ... ) so you still obtain
name_node and use source, node, sig, and push to funcs; apply the same refactor
for the type_kinds branch (the code that currently checks
type_kinds.contains(&kind) and then if let Some(name_node) = ...) so both places
eliminate the nested if and keep existing behavior.
- Around line 317-381: The import-name extractor (working on variables
clause/head/group and pushing into names) doesn't strip the TypeScript `type `
modifier, so `import type Foo` and `import { type Foo }` yield incorrect names;
update the logic in the default-import branch (the loop over head.split(',')
where you push part.to_string()) to first strip a leading "type " (e.g., part =
part.strip_prefix("type ").unwrap_or(part).trim()), and similarly in the
named-import branch (where you compute name from part or after_as) strip a
leading "type " before checking emptiness or "*" and before pushing into names;
ensure both branches trim after stripping so `type` and `type Foo` are handled
correctly.
- Around line 181-190: collect_definitions, collect_calls_in_range, and
collect_type_refs_in_range currently take 9–10 parameters; refactor by creating
small context structs (e.g., DefinitionContext, CallTraversalContext,
TypeRefContext) that bundle related input fields (node, source,
func_kinds/type_kinds/import_kinds_list, range info, etc.) and output collectors
(e.g., funcs, types, imports or a Vec field) and change each function signature
to accept the appropriate context struct(s) instead of many arguments; update
call sites to construct and pass these contexts, move any mutable collectors
into the context so they’re updated in-place, and remove the
#[allow(clippy::too_many_arguments)] annotations afterward.
In `@tests/parallel_review.rs`:
- Around line 59-70: The test parallel_json_output_valid captures the command
output into variable output but never asserts the process exit status before
parsing stdout; update the test (the Command::cargo_bin("quorum")... .output()
result handling) to assert the command exited successfully (e.g., check
output.status.success() or compare output.status.code() to the expected code)
immediately after .unwrap() and before validating/decoding output.stdout to
ensure the test fails on process errors rather than empty or invalid JSON.
🪄 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 Plus
Run ID: 5bfc2bb5-6542-4b69-a54b-d12aadd0434d
📒 Files selected for processing (105)
examples/compare_calibration.rsexamples/compare_retrieval.rssrc/agent.rssrc/analytics.rssrc/cache.rssrc/calibrate.rssrc/calibrator.rssrc/calibrator_trace.rssrc/category.rssrc/cli/mod.rssrc/cli_io.rssrc/config.rssrc/context/bootstrap.rssrc/context/cli.rssrc/context/cli_tests.rssrc/context/config.rssrc/context/config_tests.rssrc/context/extract/astgrep_hcl.rssrc/context/extract/astgrep_hcl_tests.rssrc/context/extract/astgrep_py.rssrc/context/extract/astgrep_py_tests.rssrc/context/extract/astgrep_rust.rssrc/context/extract/astgrep_rust_tests.rssrc/context/extract/astgrep_ts.rssrc/context/extract/astgrep_ts_tests.rssrc/context/extract/dispatch.rssrc/context/extract/dispatch_tests.rssrc/context/extract/markdown.rssrc/context/extract/markdown_tests.rssrc/context/index/builder.rssrc/context/index/builder_tests.rssrc/context/index/state.rssrc/context/index/state_tests.rssrc/context/inject/injector.rssrc/context/inject/plan_tests.rssrc/context/inject/render.rssrc/context/inject/stale_tests.rssrc/context/integration_tests.rssrc/context/phase2_integration_tests.rssrc/context/phase3_integration_tests.rssrc/context/phase4_integration_tests.rssrc/context/phase5_integration_tests.rssrc/context/phase6_integration_tests.rssrc/context/retrieve/bm25_tests.rssrc/context/retrieve/identifiers.rssrc/context/retrieve/identifiers_tests.rssrc/context/retrieve/mod.rssrc/context/retrieve/precedence.rssrc/context/retrieve/precedence_tests.rssrc/context/retrieve/rerank.rssrc/context/retrieve/rerank_tests.rssrc/context/retrieve/retriever.rssrc/context/retrieve/retriever_tests.rssrc/context/retrieve/structural_tests.rssrc/context/retrieve/vector.rssrc/context/retrieve/vector_tests.rssrc/context/store.rssrc/context/types_tests.rssrc/context_enrichment.rssrc/daemon.rssrc/dep_manifest.rssrc/dimensions.rssrc/domain.rssrc/embeddings.rssrc/feedback.rssrc/feedback_index.rssrc/finding.rssrc/formatting.rssrc/glyphs.rssrc/grounding.rssrc/http_server.rssrc/hydration.rssrc/linter.rssrc/llm_client.rssrc/main.rssrc/mcp/handler.rssrc/mcp/tools.rssrc/merge.rssrc/metrics.rssrc/output/mod.rssrc/parser.rssrc/patterns.rssrc/pipeline.rssrc/progress.rssrc/redact.rssrc/review.rssrc/review_log.rssrc/stats.rssrc/suppress.rssrc/telemetry.rssrc/test_support.rssrc/threshold_config.rssrc/tools.rssrc/trace_subscriber.rstests/calibrate_backfill.rstests/calibrator_pre_refactor_snapshot.rstests/category_enum_test.rstests/cli.rstests/cli_context_stdout_handling.rstests/cli_feedback_agent.rstests/cli_inbox_drain.rstests/doctor_exit_code.rstests/parallel_review.rstests/review_log.rstests/stats_dimensions.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: analyze (rust)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Do not use emojis in code or output
Support AST-based analysis for Rust files (.rs) including complexity, unsafe code blocks, and unwrap() calls
Use clippy as the linter for Rust files
Include Rust ast-grep rules (5 bundled): block-on-in-async, expect-empty-message, ignored-io-result, silent-error-conversion, string-byte-slice
Files:
src/context/integration_tests.rssrc/context/index/builder_tests.rssrc/context/store.rssrc/context/extract/astgrep_rust_tests.rssrc/context/retrieve/precedence.rssrc/context/phase2_integration_tests.rssrc/context/types_tests.rssrc/cache.rssrc/cli_io.rstests/category_enum_test.rssrc/context/retrieve/rerank_tests.rssrc/context/extract/astgrep_py.rstests/cli_context_stdout_handling.rssrc/config.rssrc/category.rstests/calibrate_backfill.rssrc/context/config_tests.rssrc/context/inject/render.rssrc/context/extract/markdown.rssrc/context/retrieve/retriever.rssrc/context/extract/dispatch_tests.rssrc/linter.rssrc/context/inject/plan_tests.rstests/cli_inbox_drain.rstests/doctor_exit_code.rssrc/context/retrieve/structural_tests.rssrc/review.rssrc/context/inject/stale_tests.rssrc/context/retrieve/mod.rssrc/embeddings.rssrc/glyphs.rssrc/context/retrieve/identifiers_tests.rssrc/test_support.rssrc/output/mod.rssrc/formatting.rssrc/merge.rssrc/context/index/state_tests.rssrc/context/extract/astgrep_ts.rstests/parallel_review.rstests/stats_dimensions.rssrc/context/retrieve/vector_tests.rssrc/context/phase4_integration_tests.rssrc/pipeline.rssrc/context/retrieve/bm25_tests.rssrc/context/phase3_integration_tests.rssrc/metrics.rssrc/cli/mod.rssrc/context/bootstrap.rssrc/context/retrieve/vector.rssrc/context/retrieve/retriever_tests.rssrc/context/index/builder.rssrc/finding.rssrc/tools.rstests/review_log.rssrc/context/extract/astgrep_hcl_tests.rssrc/context/extract/astgrep_rust.rssrc/context/extract/astgrep_ts_tests.rssrc/context/retrieve/rerank.rstests/cli.rssrc/context/config.rssrc/context/extract/markdown_tests.rstests/cli_feedback_agent.rssrc/context/extract/dispatch.rssrc/calibrate.rssrc/context/phase5_integration_tests.rstests/calibrator_pre_refactor_snapshot.rssrc/trace_subscriber.rssrc/context/inject/injector.rssrc/threshold_config.rssrc/context/extract/astgrep_hcl.rssrc/mcp/tools.rssrc/context/extract/astgrep_py_tests.rssrc/redact.rssrc/dep_manifest.rssrc/context/phase6_integration_tests.rssrc/http_server.rssrc/parser.rssrc/grounding.rssrc/calibrator_trace.rssrc/suppress.rssrc/context/index/state.rssrc/progress.rssrc/context/retrieve/precedence_tests.rssrc/context/cli.rssrc/domain.rssrc/daemon.rsexamples/compare_calibration.rsexamples/compare_retrieval.rssrc/main.rssrc/dimensions.rssrc/mcp/handler.rssrc/telemetry.rssrc/review_log.rssrc/patterns.rssrc/context/retrieve/identifiers.rssrc/stats.rssrc/agent.rssrc/hydration.rssrc/analytics.rssrc/feedback_index.rssrc/context/cli_tests.rssrc/calibrator.rssrc/feedback.rssrc/context_enrichment.rssrc/llm_client.rs
**/pipeline.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/pipeline.rs: When Context7HttpFetcher::new() fails atrun_reviewstart, setPipelineConfig.context7_disabledto true and skip per-file enrichment via thecontext7_skip_reasonpredicate insrc/pipeline.rs
Build context injector viacontext::bootstrap::build_production_injectorduringrun_reviewto retrieve top-k chunks under token budget with 40% floor, prioritizing symbols
Files:
src/pipeline.rs
**/dep_manifest.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/dep_manifest.rs: Insrc/dep_manifest.rs, when pyproject.toml[project].dependencies(PEP 621) is present and well-typed, use it over[tool.poetry.dependencies]
Insrc/dep_manifest.rs, treat pyproject.toml sections that are present but have wrong TOML type (e.g. string instead of array/table) as explicitly empty and do not fall back to requirements.txt; log a tracing::warn
Files:
src/dep_manifest.rs
**/feedback*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/feedback*.rs: Drop fp_kind when verdicts route through external agent path via--from-agentorfromAgent; log a tracing::warn when fields are dropped
Restrict external-agent verdicts to tp/fp/partial only; reject wontfix and context_misleading at the ingestion chokepoint
Normalize agent names by trimming and lowercasing when recording external verdicts
Clamp external confidence to [0,1] in a NaN-safe manner
Files:
src/feedback_index.rssrc/feedback.rs
**/calibrator.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/calibrator.rs: When feedback verdict iscontext_misleadingwithblamed_chunks, raise per-chunk injection thresholds viaCalibrator::injection_threshold_for
Permanently seal chunks (set threshold to f32::INFINITY) afterinject_suppress_afterconfirmations of context_misleading verdict
Files:
src/calibrator.rs
**/context_enrichment.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/context_enrichment.rs: Insrc/context_enrichment.rs::enrich_for_review_in_project, parse project dependency manifests (Cargo.toml, package.json, pyproject.toml with requirements.txt fallback) and filter to at most K=5 deps whose names appear in the file's import_targets
Query Context7 insrc/context_enrichment.rswith curated queries viacurated_query_for(name)or language-aware generic queries viageneric_query_for_language(lang)
Files:
src/context_enrichment.rs
**/context*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Cache Context7 resolve results with 24h TTL including negative results, with clock injectable via
CachedContextFetcher::new_with_clockfor testability
Files:
src/context_enrichment.rs
**/llm_client.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/llm_client.rs: The base_url validator insrc/llm_client.rs::validate_base_urlmust require HTTPS by default, reject credentials in URL, block private/loopback IPs, and consult QUORUM_ALLOWED_BASE_URL_HOSTS
Insrc/llm_client.rs::validate_base_url, the scheme check must key on whether the resolved host is actually private, not on the env var alone, to prevent QUORUM_ALLOW_PRIVATE_BASE_URL bypass
Files:
src/llm_client.rs
🧠 Learnings (1)
📚 Learning: 2026-05-06T02:37:09.757Z
Learnt from: jsnyder
Repo: jsnyder/quorum PR: 235
File: src/dimensions.rs:408-408
Timestamp: 2026-05-06T02:37:09.757Z
Learning: Do not flag indentation-only differences in Rust struct initializers as syntax errors. Indentation variations are cosmetic and handled by rustfmt. Only report real syntax/semantic issues (e.g., missing commas between fields, incorrect field names/types, or actual compile errors). Apply this guidance to all Rust source files in the repo.
Applied to files:
src/context/integration_tests.rssrc/context/index/builder_tests.rssrc/context/store.rssrc/context/extract/astgrep_rust_tests.rssrc/context/retrieve/precedence.rssrc/context/phase2_integration_tests.rssrc/context/types_tests.rssrc/cache.rssrc/cli_io.rstests/category_enum_test.rssrc/context/retrieve/rerank_tests.rssrc/context/extract/astgrep_py.rstests/cli_context_stdout_handling.rssrc/config.rssrc/category.rstests/calibrate_backfill.rssrc/context/config_tests.rssrc/context/inject/render.rssrc/context/extract/markdown.rssrc/context/retrieve/retriever.rssrc/context/extract/dispatch_tests.rssrc/linter.rssrc/context/inject/plan_tests.rstests/cli_inbox_drain.rstests/doctor_exit_code.rssrc/context/retrieve/structural_tests.rssrc/review.rssrc/context/inject/stale_tests.rssrc/context/retrieve/mod.rssrc/embeddings.rssrc/glyphs.rssrc/context/retrieve/identifiers_tests.rssrc/test_support.rssrc/output/mod.rssrc/formatting.rssrc/merge.rssrc/context/index/state_tests.rssrc/context/extract/astgrep_ts.rstests/parallel_review.rstests/stats_dimensions.rssrc/context/retrieve/vector_tests.rssrc/context/phase4_integration_tests.rssrc/pipeline.rssrc/context/retrieve/bm25_tests.rssrc/context/phase3_integration_tests.rssrc/metrics.rssrc/cli/mod.rssrc/context/bootstrap.rssrc/context/retrieve/vector.rssrc/context/retrieve/retriever_tests.rssrc/context/index/builder.rssrc/finding.rssrc/tools.rstests/review_log.rssrc/context/extract/astgrep_hcl_tests.rssrc/context/extract/astgrep_rust.rssrc/context/extract/astgrep_ts_tests.rssrc/context/retrieve/rerank.rstests/cli.rssrc/context/config.rssrc/context/extract/markdown_tests.rstests/cli_feedback_agent.rssrc/context/extract/dispatch.rssrc/calibrate.rssrc/context/phase5_integration_tests.rstests/calibrator_pre_refactor_snapshot.rssrc/trace_subscriber.rssrc/context/inject/injector.rssrc/threshold_config.rssrc/context/extract/astgrep_hcl.rssrc/mcp/tools.rssrc/context/extract/astgrep_py_tests.rssrc/redact.rssrc/dep_manifest.rssrc/context/phase6_integration_tests.rssrc/http_server.rssrc/parser.rssrc/grounding.rssrc/calibrator_trace.rssrc/suppress.rssrc/context/index/state.rssrc/progress.rssrc/context/retrieve/precedence_tests.rssrc/context/cli.rssrc/domain.rssrc/daemon.rsexamples/compare_calibration.rsexamples/compare_retrieval.rssrc/main.rssrc/dimensions.rssrc/mcp/handler.rssrc/telemetry.rssrc/review_log.rssrc/patterns.rssrc/context/retrieve/identifiers.rssrc/stats.rssrc/agent.rssrc/hydration.rssrc/analytics.rssrc/feedback_index.rssrc/context/cli_tests.rssrc/calibrator.rssrc/feedback.rssrc/context_enrichment.rssrc/llm_client.rs
🪛 GitHub Actions: CI / 3_clippy.txt
src/calibrate.rs
[error] 215-215: Very complex type used. Consider factoring parts into type definitions (clippy: type-complexity)
[error] 347-351: this if statement can be collapsed (collapsible_if)
src/domain.rs
[error] 403-403: Duplicated attribute #[test] (duplicate-macro-attributes)
src/hydration.rs
[error] 181-181: this function has too many arguments (8/7) (clippy: too_many_arguments)
[error] 393-393: called Iterator::last on a DoubleEndedIterator (clippy: double_ended_iterator_last)
[error] 616-616: stripping a prefix manually; consider using strip_prefix (clippy: manual_strip)
src/calibrator.rs
[error] 3031-3031: Accessing first element with get(0); consider using first() (get_first)
[error] 2203-2203: Useless use of vec!; consider using a slice directly (&[...] ). (useless_vec)
[error] 2212-2212: Useless use of vec!; consider using a slice directly (&[...] ). (useless_vec)
src/feedback.rs
[error] 1372-1372: CLippy: manual_contains; this map_or can be simplified; use is_none_or or similar
[error] 2187-2187: repeat().take() can be written more concisely; consider using repeat_n()
🪛 GitHub Actions: CI / clippy
src/calibrate.rs
[error] 215-215: this if statement can be collapsed
[error] 347-347: this if statement can be collapsed
[error] 355-355: this if statement can be collapsed
src/domain.rs
[error] 403-403: duplicated attribute
src/hydration.rs
[error] 181-181: this function has too many arguments (8/7)
[error] 195-195: this if statement can be collapsed
[error] 616-616: stripping a prefix manually; use strip_prefix() instead
[error] 403-403: this function has too many arguments (10/7)
[error] 466-466: this function has too many arguments (9/7)
[error] 393-393: called Iterator::last on a DoubleEndedIterator; this will iterate the entire iterator
src/analytics.rs
[warning] 393-393: called Iterator::last on a DoubleEndedIterator; this will iterate the entire iterator
src/calibrator.rs
[error] 2203-2203: useless use of vec!
[error] 2212-2212: useless use of vec!
[error] 2261-2261: useless use of vec!
[error] 2268-2268: useless use of vec!
[error] 2295-2295: useless use of vec!
src/feedback.rs
[error] 1372-1372: this map_or can be simplified
[warning] 2187-2187: repeat().take() can be written more concisely
| let result1 = calibrate( | ||
| findings.clone(), | ||
| &vec![human_fp.clone(), auto_fp.clone()], | ||
| &config, | ||
| "", | ||
| ); | ||
| assert_eq!(result1.suppressed, 1, "Human+auto FP should suppress"); | ||
|
|
||
| // 2 auto only: 0.5 + 0.5 = 1.0 < 1.5 threshold -> NOT suppress | ||
| let result2 = calibrate(findings.clone(), &vec![auto_fp.clone(), auto_fp], &config, ""); | ||
| assert_eq!(result2.suppressed, 0, "2 auto FPs alone should not suppress (insufficient weight)"); | ||
| let result2 = calibrate( | ||
| findings.clone(), | ||
| &vec![auto_fp.clone(), auto_fp], | ||
| &config, | ||
| "", | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the structure and find the calibrate function
cd /tmp/codebase || cd /
fd "calibrator.rs" --type fRepository: jsnyder/quorum
Length of output: 176
🏁 Script executed:
# Get the calibrate function signature
rg "fn calibrate" --type rust -A 5Repository: jsnyder/quorum
Length of output: 13575
🏁 Script executed:
# Check lines 2201-2215
sed -n '2201,2215p' src/calibrator.rsRepository: jsnyder/quorum
Length of output: 528
🏁 Script executed:
# Check lines 2259-2268
sed -n '2259,2268p' src/calibrator.rsRepository: jsnyder/quorum
Length of output: 439
🏁 Script executed:
# Check line 2295
sed -n '2295p' src/calibrator.rsRepository: jsnyder/quorum
Length of output: 124
Replace &vec![...] with &[...] to resolve the useless_vec clippy lint.
The calibrate() function signature accepts feedback: &[FeedbackEntry], so passing a slice literal is more efficient and idiomatic than constructing a vector and borrowing it.
Proposed fix
- let result1 = calibrate(
- findings.clone(),
- &vec![human_fp.clone(), auto_fp.clone()],
- &config,
- "",
- );
+ let result1 = calibrate(
+ findings.clone(),
+ &[human_fp.clone(), auto_fp.clone()],
+ &config,
+ "",
+ );
- let result2 = calibrate(
- findings.clone(),
- &vec![auto_fp.clone(), auto_fp],
- &config,
- "",
- );
+ let result2 = calibrate(
+ findings.clone(),
+ &[auto_fp.clone(), auto_fp],
+ &config,
+ "",
+ );
- let result1 = calibrate(
- findings.clone(),
- &vec![recent_fp.clone(), recent_fp],
- &config,
- "",
- );
+ let result1 = calibrate(
+ findings.clone(),
+ &[recent_fp.clone(), recent_fp],
+ &config,
+ "",
+ );
- let result2 = calibrate(findings.clone(), &vec![old_fp.clone(), old_fp], &config, "");
+ let result2 = calibrate(findings.clone(), &[old_fp.clone(), old_fp], &config, "");
- let result = calibrate(findings, &vec![tp], &config, "");
+ let result = calibrate(findings, &[tp], &config, "");Also applies to: lines 2259-2268, 2295
🧰 Tools
🪛 GitHub Actions: CI / 3_clippy.txt
[error] 2203-2203: Useless use of vec!; consider using a slice directly (&[...] ). (useless_vec)
[error] 2212-2212: Useless use of vec!; consider using a slice directly (&[...] ). (useless_vec)
🪛 GitHub Actions: CI / clippy
[error] 2203-2203: useless use of vec!
[error] 2212-2212: useless use of vec!
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/calibrator.rs` around lines 2201 - 2215, The calls to calibrate (e.g.,
the call computing result1 and result2 and the other mentions around lines
2259-2268 and 2295) are passing &vec![...] which triggers the useless_vec clippy
lint; change those borrowed Vec constructions to slice literals by passing
&[human_fp.clone(), auto_fp.clone()] (or &[auto_fp.clone(), auto_fp]) instead of
&vec![...], ensuring the argument type still matches calibrate(feedback:
&[FeedbackEntry]) and preserving clones where needed.
There was a problem hiding this comment.
Pre-existing code, not introduced by this formatting PR. Will triage separately.
There was a problem hiding this comment.
@jsnyder, understood — the &vec![...] pattern was already present before this formatting sweep and is out of scope for a rustfmt-only PR. Would you like me to open a follow-up GitHub issue to track the useless_vec clippy lint fixes in src/calibrator.rs?
🧠 Learnings used
Learnt from: jsnyder
Repo: jsnyder/quorum PR: 235
File: src/dimensions.rs:408-408
Timestamp: 2026-05-06T02:37:09.757Z
Learning: Do not flag indentation-only differences in Rust struct initializers as syntax errors. Indentation variations are cosmetic and handled by rustfmt. Only report real syntax/semantic issues (e.g., missing commas between fields, incorrect field names/types, or actual compile errors). Apply this guidance to all Rust source files in the repo.
| eprintln!( | ||
| "output severity: {:?} (input High)\n", | ||
| result.findings.get(0).map(|f| f.severity.clone()) | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the file exists and get its size
wc -l src/calibrator.rsRepository: jsnyder/quorum
Length of output: 81
🏁 Script executed:
# Read the specific lines mentioned in the review (3029-3032)
sed -n '3029,3032p' src/calibrator.rsRepository: jsnyder/quorum
Length of output: 203
🏁 Script executed:
# Also get some context around those lines
sed -n '3025,3035p' src/calibrator.rsRepository: jsnyder/quorum
Length of output: 449
🏁 Script executed:
# Search for clippy configuration
find . -type f \( -name "clippy.toml" -o -name ".clippy.toml" \) 2>/dev/null | head -20Repository: jsnyder/quorum
Length of output: 40
🏁 Script executed:
# Check for CI configuration that might mention this lint
find . -type f \( -name "*.yml" -o -name "*.yaml" \) | xargs grep -l "clippy\|get_first" 2>/dev/null | head -10Repository: jsnyder/quorum
Length of output: 85
🏁 Script executed:
# Find the definition of result.findings to verify it's a Vec/slice
rg "findings\s*:" src/calibrator.rs -B2 -A2 | head -30Repository: jsnyder/quorum
Length of output: 688
🏁 Script executed:
# Look for the result struct definition
rg "struct.*Result\|findings.*Vec\|findings.*\[" src/calibrator.rs | head -20Repository: jsnyder/quorum
Length of output: 40
🏁 Script executed:
# Check the CI workflow configuration
cat .github/workflows/ci.yml | head -100Repository: jsnyder/quorum
Length of output: 2548
🏁 Script executed:
# Search for clippy invocations or linting steps in CI
rg "clippy|lint" .github/workflows/ci.yml -iRepository: jsnyder/quorum
Length of output: 261
Use .first() instead of .get(0) to satisfy the clippy get_first lint.
The CI job runs clippy with -D warnings, and this pattern triggers the standard get_first recommendation. Replace result.findings.get(0) with result.findings.first().
Proposed fix
- eprintln!(
- "output severity: {:?} (input High)\n",
- result.findings.get(0).map(|f| f.severity.clone())
- );
+ eprintln!(
+ "output severity: {:?} (input High)\n",
+ result.findings.first().map(|f| f.severity.clone())
+ );📝 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.
| eprintln!( | |
| "output severity: {:?} (input High)\n", | |
| result.findings.get(0).map(|f| f.severity.clone()) | |
| ); | |
| eprintln!( | |
| "output severity: {:?} (input High)\n", | |
| result.findings.first().map(|f| f.severity.clone()) | |
| ); |
🧰 Tools
🪛 GitHub Actions: CI / 3_clippy.txt
[error] 3031-3031: Accessing first element with get(0); consider using first() (get_first)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/calibrator.rs` around lines 3029 - 3032, Replace the use of Vec::get(0)
with Vec::first() on the findings vector to satisfy the clippy get_first lint:
update the eprintln call that currently uses result.findings.get(0) to use
result.findings.first() (preserving the map/Option handling and cloning of
severity) so the printed behavior remains identical while removing the lint
warning in the eprintln! invocation that references result.findings.
There was a problem hiding this comment.
Pre-existing code, not introduced by this formatting PR. Will triage separately.
There was a problem hiding this comment.
@jsnyder Understood — the result.findings.get(0) pattern predates this PR and the formatting sweep just reflowed it. Would you like me to open a follow-up GitHub issue to track the get_first clippy lint fix so it doesn't get lost?
🧠 Learnings used
Learnt from: jsnyder
Repo: jsnyder/quorum PR: 235
File: src/dimensions.rs:408-408
Timestamp: 2026-05-06T02:37:09.757Z
Learning: Do not flag indentation-only differences in Rust struct initializers as syntax errors. Indentation variations are cosmetic and handled by rustfmt. Only report real syntax/semantic issues (e.g., missing commas between fields, incorrect field names/types, or actual compile errors). Apply this guidance to all Rust source files in the repo.
| assert!( | ||
| inner.get("confidence").map_or(true, |c| c.is_null()), | ||
| "confidence must be null or absent, got {:?}", | ||
| inner.get("confidence") | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if src/feedback.rs exists and read the relevant lines
head -1375 src/feedback.rs | tail -10Repository: jsnyder/quorum
Length of output: 547
🏁 Script executed:
# Check Rust version constraints in Cargo.toml
cat Cargo.toml | grep -A 5 "rust-version\|edition"Repository: jsnyder/quorum
Length of output: 581
🏁 Script executed:
# Search for any clippy configuration or MSRV hints
find . -name "clippy.toml" -o -name ".clippy.toml" -o -name "rust-toolchain.toml" | head -5Repository: jsnyder/quorum
Length of output: 40
🏁 Script executed:
# Search for Clippy lints related to map_or
rg "map_or" --type rust -A 2 -B 2 src/feedback.rs | head -20Repository: jsnyder/quorum
Length of output: 319
🏁 Script executed:
# Check if is_none_or is used elsewhere in the codebase
rg "is_none_or" --type rustRepository: jsnyder/quorum
Length of output: 194
🌐 Web query:
Rust is_none_or stabilized version when available
💡 Result:
In stable Rust, the API is Option::is_none_or. It was stabilized as part of Rust 1.82.0: the stabilization PR for is_none_or targets the 1.82.0 milestone [1], and the std::option::Option docs show is_none_or available in 1.82.0 (marked “unstable” on the page, reflecting documentation source metadata) [2]. How to use (stable): opt.is_none_or(|v| /* predicate on v */) returns true if opt is None or if opt is Some(v) and the predicate returns true [2]. Note: before stabilization it was behind the nightly feature gate #![feature(is_none_or)], as shown by the tracking issue [3]. So the answer to “Rust is_none_or stabilized version when available” is: stable from Rust 1.82.0 onward [1][2].
Citations:
- 1: Stabilize
is_none_orrust-lang/rust#129086 - 2: https://doc.rust-lang.org/beta/std/option/enum.Option.html
- 3: Tracking Issue for is_none_or rust-lang/rust#126383
Unblock the Clippy job by simplifying this Option check.
This assertion is failing CI; map_or(true, |c| c.is_null()) should use the more idiomatic is_none_or(|c| c.is_null()) instead to pass the Clippy gates. Your project already uses is_none_or elsewhere (src/grounding.rs), and MSRV 1.93 fully supports it (stabilized in 1.82.0).
Suggested fix
assert!(
- inner.get("confidence").map_or(true, |c| c.is_null()),
+ inner.get("confidence").is_none_or(|c| c.is_null()),
"confidence must be null or absent, got {:?}",
inner.get("confidence")
);🧰 Tools
🪛 GitHub Actions: CI / 3_clippy.txt
[error] 1372-1372: CLippy: manual_contains; this map_or can be simplified; use is_none_or or similar
🪛 GitHub Actions: CI / clippy
[error] 1372-1372: this map_or can be simplified
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/feedback.rs` around lines 1371 - 1375, The assertion on
inner.get("confidence") uses map_or(true, |c| c.is_null()), which Clippy flags;
replace that check with the idiomatic is_none_or(|c| c.is_null()) on
inner.get("confidence") in the assert! so it reads that the "confidence" entry
must be none or null while keeping the existing error message and location (the
assert! around inner.get("confidence")).
There was a problem hiding this comment.
Pre-existing code, not introduced by this formatting PR. Will triage separately.
There was a problem hiding this comment.
@jsnyder, understood — the Clippy lint on map_or(true, |c| c.is_null()) predates this formatting sweep and is out of scope here. Would you like me to open a follow-up GitHub issue to track the is_none_or migration so it doesn't get lost?
🧠 Learnings used
Learnt from: jsnyder
Repo: jsnyder/quorum PR: 235
File: src/dimensions.rs:408-408
Timestamp: 2026-05-06T02:37:09.757Z
Learning: Do not flag indentation-only differences in Rust struct initializers as syntax errors. Indentation variations are cosmetic and handled by rustfmt. Only report real syntax/semantic issues (e.g., missing commas between fields, incorrect field names/types, or actual compile errors). Apply this guidance to all Rust source files in the repo.
| pub fn detect() -> Self { | ||
| Self { is_tty: io::stderr().is_terminal() } | ||
| Self { | ||
| is_tty: io::stderr().is_terminal(), | ||
| } | ||
| } |
There was a problem hiding this comment.
Keep this PR formatting-only by moving ProgressReporter::detect() to a separate PR.
detect() is a new public API and functional addition, which conflicts with the stated formatting-only scope for this sweep.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/progress.rs` around lines 19 - 23, Remove the new public API method
ProgressReporter::detect() from this formatting-only change: delete the detect()
function declaration/implementation in src/progress.rs (the self.is_tty =
io::stderr().is_terminal() logic) and revert any call sites introduced in this
PR so the behavior remains unchanged; instead, extract that method and its
tests/uses into a separate focused PR for the functional addition and optionally
leave a brief comment referencing the new PR for future work.
There was a problem hiding this comment.
Pre-existing code, not introduced by this formatting PR. Will triage separately.
There was a problem hiding this comment.
@jsnyder, understood — my apologies for the noise. The diff only reformats the struct initializer inside detect() from single-line to multi-line, and detect() itself predates this PR. I incorrectly flagged it as a new addition. I'll note this to avoid similar false positives in future formatting sweeps.
✏️ Learnings added
Learnt from: jsnyder
Repo: jsnyder/quorum PR: 236
File: src/progress.rs:19-23
Timestamp: 2026-05-06T12:59:14.821Z
Learning: In the jsnyder/quorum repo, ProgressReporter::detect() in src/progress.rs is a pre-existing public method (not introduced by the fmt-sweep PR `#236`). Formatting-only PRs in this repo may reflow pre-existing code; do not flag pre-existing methods or fields as new functional additions solely based on their appearance in a formatting diff.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: jsnyder
Repo: jsnyder/quorum PR: 235
File: src/dimensions.rs:408-408
Timestamp: 2026-05-06T02:37:09.757Z
Learning: Do not flag indentation-only differences in Rust struct initializers as syntax errors. Indentation variations are cosmetic and handled by rustfmt. Only report real syntax/semantic issues (e.g., missing commas between fields, incorrect field names/types, or actual compile errors). Apply this guidance to all Rust source files in the repo.
1 participant
Summary
cargo fmt --allto the entire codebase — catches up on accumulated formatting driftCloses #216
Test plan
cargo fmt --all -- --checkpasses (zero diff)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests