From ceac70ddd21b2c497efa52bab637caa3cf2dce09 Mon Sep 17 00:00:00 2001 From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com> Date: Sat, 16 May 2026 15:36:42 +0100 Subject: [PATCH] fix(shared-context): clear pre-existing clippy -D warnings debt (item B) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ~20 mechanical clippy lib lints in `gitbot-shared-context`, all fixed idiomatically with no API/behaviour change (verified: build + clippy --lib --tests --bins -D warnings clean + 84 tests pass): - context.rs: drop unused `Severity` import; derive `Default` for ContextConfig instead of hand-impl. - reporting.rs: trim unused imports (move test-only `BotId` into the test module); `push('\n')`; iterate map `.values()`. - storage.rs: drop unused `SessionState`; collapse nested ifs; `sort_by_key(Reverse(..))`; `#[cfg(test)] Default for MemoryStorage`; `#[allow(should_implement_trait)]` + rationale on fallible `default()`. - bot.rs: `#[allow(should_implement_trait)]` + rationale on `from_str` -> Option; remove if/else with identical blocks. - exclusion_registry.rs: implement real `std::str::FromStr` (signature-compatible) + update call sites to `.parse()`; `if let Ok(..) = env::var(..)`; collapse nested if. - registry_guard.rs: collapse nested if; manual `split_once`. Scope strictly shared-context/src/; benches/ untouched (handled in #145). Disjoint from #145 — independent PR for separate review. Co-Authored-By: Claude Opus 4.7 (1M context) --- shared-context/src/bot.rs | 9 +++-- shared-context/src/context.rs | 17 ++-------- shared-context/src/exclusion_registry.rs | 23 ++++++++----- shared-context/src/registry_guard.rs | 12 +++---- shared-context/src/reporting.rs | 7 ++-- shared-context/src/storage.rs | 43 ++++++++++++++---------- 6 files changed, 55 insertions(+), 56 deletions(-) diff --git a/shared-context/src/bot.rs b/shared-context/src/bot.rs index 1752127..3e48631 100644 --- a/shared-context/src/bot.rs +++ b/shared-context/src/bot.rs @@ -84,6 +84,8 @@ impl BotId { } /// Parse from string + // Returns `Option`, not `Result`, so the std `FromStr` trait does not fit; keep the name as part of the public API. + #[allow(clippy::should_implement_trait)] pub fn from_str(s: &str) -> Option { match s.to_lowercase().as_str() { "rhodibot" => Some(BotId::Rhodibot), @@ -429,11 +431,8 @@ impl BotExecution { /// Mark as completed pub fn complete(&mut self, findings: usize, errors: usize, files: usize) { let now = chrono::Utc::now(); - self.status = if errors > 0 { - BotStatus::Completed // Has errors but completed - } else { - BotStatus::Completed - }; + // Completed regardless of whether errors were reported. + self.status = BotStatus::Completed; self.completed_at = Some(now); self.findings_count = findings; self.errors_count = errors; diff --git a/shared-context/src/context.rs b/shared-context/src/context.rs index 18cb9eb..884df0c 100644 --- a/shared-context/src/context.rs +++ b/shared-context/src/context.rs @@ -2,7 +2,7 @@ //! Shared context for coordinating bot executions use crate::bot::{BotExecution, BotId, BotStatus, Tier}; -use crate::finding::{Finding, FindingSet, Severity}; +use crate::finding::{Finding, FindingSet}; use crate::Result; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; @@ -250,7 +250,7 @@ impl Context { } /// Context configuration -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Default, Serialize, Deserialize)] pub struct ContextConfig { /// Enable dry-run mode (no changes) pub dry_run: bool, @@ -266,19 +266,6 @@ pub struct ContextConfig { pub bot_config: HashMap, } -impl Default for ContextConfig { - fn default() -> Self { - Self { - dry_run: false, - auto_fix: false, - strict: false, - skip_bots: Vec::new(), - skip_categories: Vec::new(), - bot_config: HashMap::new(), - } - } -} - /// Summary of context execution #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ContextSummary { diff --git a/shared-context/src/exclusion_registry.rs b/shared-context/src/exclusion_registry.rs index fbe5fb2..8fe35e1 100644 --- a/shared-context/src/exclusion_registry.rs +++ b/shared-context/src/exclusion_registry.rs @@ -30,6 +30,7 @@ use std::env; use std::path::{Path, PathBuf}; +use std::str::FromStr; use glob::Pattern; use serde::Deserialize; @@ -153,11 +154,15 @@ impl ExclusionRegistry { e )) })?; - Self::from_str(&source) + source.parse() } +} + +impl FromStr for ExclusionRegistry { + type Err = ExclusionError; /// Parse from an in-memory A2ML string. - pub fn from_str(source: &str) -> Result { + fn from_str(source: &str) -> Result { let raw: RawRegistry = toml::from_str(source).map_err(|e| { ExclusionError::Parse(format!("failed to parse exclusion registry: {e}")) })?; @@ -215,7 +220,9 @@ impl ExclusionRegistry { remote_origin_patterns, }) } +} +impl ExclusionRegistry { /// Load from the location specified by `BOT_EXCLUSION_REGISTRY` env, else /// the first of a set of conventional locations that exists. /// @@ -288,7 +295,7 @@ impl ExclusionRegistry { } // Kill-switch check first — cheapest, catches everything. - if let Some(kill) = env::var("HYPATIA_AUTOMATION").ok() { + if let Ok(kill) = env::var("HYPATIA_AUTOMATION") { let k = kill.to_ascii_lowercase(); if matches!(k.as_str(), "off" | "disabled" | "0" | "false" | "halt") { return Decision::Deny { @@ -358,10 +365,10 @@ fn normalise_origin(origin: &str) -> String { // Strip trailing .git let s = s.strip_suffix(".git").unwrap_or(s); // git@host:owner/repo → host/owner/repo - if let Some(rest) = s.strip_prefix("git@") { - if let Some((host, path)) = rest.split_once(':') { - return format!("{host}/{path}"); - } + if let Some(rest) = s.strip_prefix("git@") + && let Some((host, path)) = rest.split_once(':') + { + return format!("{host}/{path}"); } // https://host/path or http://host/path or ssh://host/path for prefix in ["https://", "http://", "ssh://", "git://"] { @@ -468,7 +475,7 @@ reason = "upstream homebrew" "#; fn registry() -> ExclusionRegistry { - ExclusionRegistry::from_str(FIXTURE).unwrap() + FIXTURE.parse().unwrap() } #[test] diff --git a/shared-context/src/registry_guard.rs b/shared-context/src/registry_guard.rs index 3f5a3d5..8b2c12d 100644 --- a/shared-context/src/registry_guard.rs +++ b/shared-context/src/registry_guard.rs @@ -137,19 +137,17 @@ fn parse_full_name(url: &str) -> Option { let s = url.strip_suffix(".git").unwrap_or(url); // git@host:owner/repo - if let Some(rest) = s.strip_prefix("git@") { - if let Some((_host, path)) = rest.split_once(':') { - return Some(path.to_string()); - } + if let Some(rest) = s.strip_prefix("git@") + && let Some((_host, path)) = rest.split_once(':') + { + return Some(path.to_string()); } // https://host/owner/repo or ssh://host/owner/repo for prefix in ["https://", "http://", "ssh://", "git://"] { if let Some(rest) = s.strip_prefix(prefix) { // Skip the host segment. - let mut parts = rest.splitn(2, '/'); - let _host = parts.next()?; - let path = parts.next()?; + let (_host, path) = rest.split_once('/')?; return Some(path.to_string()); } } diff --git a/shared-context/src/reporting.rs b/shared-context/src/reporting.rs index 450baa4..84e25d7 100644 --- a/shared-context/src/reporting.rs +++ b/shared-context/src/reporting.rs @@ -4,7 +4,7 @@ //! Generates reports about fleet execution, findings, and bot performance //! across all registered bots. Supports multiple output formats. -use crate::{BotId, Context, Finding, Severity, Tier}; +use crate::{Context, Severity, Tier}; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; use std::collections::HashMap; @@ -290,13 +290,13 @@ impl Context { .unwrap_or_else(|| "-".to_string()) )); } - md.push_str("\n"); + md.push('\n'); // Tier Performance md.push_str("## Tier Performance\n\n"); md.push_str("| Tier | Bots | Completed | Findings | Avg Duration (ms) |\n"); md.push_str("|------|------|-----------|----------|-------------------|\n"); - for (_, perf) in &report.tier_performance { + for perf in report.tier_performance.values() { md.push_str(&format!( "| {} | {} | {} | {} | {:.0} |\n", perf.tier, perf.bots_count, perf.completed_count, perf.total_findings, perf.avg_duration_ms @@ -362,6 +362,7 @@ impl Context { #[cfg(test)] mod tests { use super::*; + use crate::BotId; use std::path::PathBuf; #[test] diff --git a/shared-context/src/storage.rs b/shared-context/src/storage.rs index f5e524d..e7fe867 100644 --- a/shared-context/src/storage.rs +++ b/shared-context/src/storage.rs @@ -2,7 +2,7 @@ //! Context storage backends use crate::context::Context; -use crate::state::{RepoState, SessionState}; +use crate::state::RepoState; use crate::{ContextError, Result}; use std::path::{Path, PathBuf}; use tracing::{debug, info}; @@ -22,6 +22,8 @@ impl ContextStorage { } /// Create storage in default location (~/.gitbot-fleet) + // Fallible (`Result`), so it cannot implement the infallible std `Default` trait; name kept as public API. + #[allow(clippy::should_implement_trait)] pub fn default() -> Result { let home = std::env::var("HOME").map_err(|_| { ContextError::InvalidState("HOME environment variable not set".to_string()) @@ -86,12 +88,11 @@ impl ContextStorage { for entry in std::fs::read_dir(&sessions_dir)? { let entry = entry?; let path = entry.path(); - if path.extension().map(|e| e == "json").unwrap_or(false) { - if let Some(stem) = path.file_stem().and_then(|s| s.to_str()) { - if let Ok(id) = uuid::Uuid::parse_str(stem) { - sessions.push(id); - } - } + if path.extension().map(|e| e == "json").unwrap_or(false) + && let Some(stem) = path.file_stem().and_then(|s| s.to_str()) + && let Ok(id) = uuid::Uuid::parse_str(stem) + { + sessions.push(id); } } @@ -152,10 +153,10 @@ impl ContextStorage { for entry in std::fs::read_dir(&repos_dir)? { let entry = entry?; let path = entry.path(); - if path.extension().map(|e| e == "json").unwrap_or(false) { - if let Some(stem) = path.file_stem().and_then(|s| s.to_str()) { - repos.push(stem.to_string()); - } + if path.extension().map(|e| e == "json").unwrap_or(false) + && let Some(stem) = path.file_stem().and_then(|s| s.to_str()) + { + repos.push(stem.to_string()); } } @@ -174,17 +175,16 @@ impl ContextStorage { for entry in std::fs::read_dir(&sessions_dir)? { let entry = entry?; let path = entry.path(); - if path.extension().map(|e| e == "json").unwrap_or(false) { - if let Ok(metadata) = entry.metadata() { - if let Ok(modified) = metadata.modified() { - sessions.push((path, modified)); - } - } + if path.extension().map(|e| e == "json").unwrap_or(false) + && let Ok(metadata) = entry.metadata() + && let Ok(modified) = metadata.modified() + { + sessions.push((path, modified)); } } // Sort by modification time (newest first) - sessions.sort_by(|a, b| b.1.cmp(&a.1)); + sessions.sort_by_key(|b| std::cmp::Reverse(b.1)); // Delete sessions beyond keep limit let mut deleted = 0; @@ -223,6 +223,13 @@ pub struct MemoryStorage { repos: std::collections::HashMap, } +#[cfg(test)] +impl Default for MemoryStorage { + fn default() -> Self { + Self::new() + } +} + #[cfg(test)] impl MemoryStorage { pub fn new() -> Self {