Skip to content

Commit ceac70d

Browse files
hyperpolymathclaude
andcommitted
fix(shared-context): clear pre-existing clippy -D warnings debt (item B)
~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) <noreply@anthropic.com>
1 parent 6426272 commit ceac70d

6 files changed

Lines changed: 55 additions & 56 deletions

File tree

shared-context/src/bot.rs

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,8 @@ impl BotId {
8484
}
8585

8686
/// Parse from string
87+
// Returns `Option`, not `Result`, so the std `FromStr` trait does not fit; keep the name as part of the public API.
88+
#[allow(clippy::should_implement_trait)]
8789
pub fn from_str(s: &str) -> Option<BotId> {
8890
match s.to_lowercase().as_str() {
8991
"rhodibot" => Some(BotId::Rhodibot),
@@ -429,11 +431,8 @@ impl BotExecution {
429431
/// Mark as completed
430432
pub fn complete(&mut self, findings: usize, errors: usize, files: usize) {
431433
let now = chrono::Utc::now();
432-
self.status = if errors > 0 {
433-
BotStatus::Completed // Has errors but completed
434-
} else {
435-
BotStatus::Completed
436-
};
434+
// Completed regardless of whether errors were reported.
435+
self.status = BotStatus::Completed;
437436
self.completed_at = Some(now);
438437
self.findings_count = findings;
439438
self.errors_count = errors;

shared-context/src/context.rs

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
//! Shared context for coordinating bot executions
33
44
use crate::bot::{BotExecution, BotId, BotStatus, Tier};
5-
use crate::finding::{Finding, FindingSet, Severity};
5+
use crate::finding::{Finding, FindingSet};
66
use crate::Result;
77
use chrono::{DateTime, Utc};
88
use serde::{Deserialize, Serialize};
@@ -250,7 +250,7 @@ impl Context {
250250
}
251251

252252
/// Context configuration
253-
#[derive(Debug, Clone, Serialize, Deserialize)]
253+
#[derive(Debug, Clone, Default, Serialize, Deserialize)]
254254
pub struct ContextConfig {
255255
/// Enable dry-run mode (no changes)
256256
pub dry_run: bool,
@@ -266,19 +266,6 @@ pub struct ContextConfig {
266266
pub bot_config: HashMap<BotId, serde_json::Value>,
267267
}
268268

269-
impl Default for ContextConfig {
270-
fn default() -> Self {
271-
Self {
272-
dry_run: false,
273-
auto_fix: false,
274-
strict: false,
275-
skip_bots: Vec::new(),
276-
skip_categories: Vec::new(),
277-
bot_config: HashMap::new(),
278-
}
279-
}
280-
}
281-
282269
/// Summary of context execution
283270
#[derive(Debug, Clone, Serialize, Deserialize)]
284271
pub struct ContextSummary {

shared-context/src/exclusion_registry.rs

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
3131
use std::env;
3232
use std::path::{Path, PathBuf};
33+
use std::str::FromStr;
3334

3435
use glob::Pattern;
3536
use serde::Deserialize;
@@ -153,11 +154,15 @@ impl ExclusionRegistry {
153154
e
154155
))
155156
})?;
156-
Self::from_str(&source)
157+
source.parse()
157158
}
159+
}
160+
161+
impl FromStr for ExclusionRegistry {
162+
type Err = ExclusionError;
158163

159164
/// Parse from an in-memory A2ML string.
160-
pub fn from_str(source: &str) -> Result<Self> {
165+
fn from_str(source: &str) -> Result<Self> {
161166
let raw: RawRegistry = toml::from_str(source).map_err(|e| {
162167
ExclusionError::Parse(format!("failed to parse exclusion registry: {e}"))
163168
})?;
@@ -215,7 +220,9 @@ impl ExclusionRegistry {
215220
remote_origin_patterns,
216221
})
217222
}
223+
}
218224

225+
impl ExclusionRegistry {
219226
/// Load from the location specified by `BOT_EXCLUSION_REGISTRY` env, else
220227
/// the first of a set of conventional locations that exists.
221228
///
@@ -288,7 +295,7 @@ impl ExclusionRegistry {
288295
}
289296

290297
// Kill-switch check first — cheapest, catches everything.
291-
if let Some(kill) = env::var("HYPATIA_AUTOMATION").ok() {
298+
if let Ok(kill) = env::var("HYPATIA_AUTOMATION") {
292299
let k = kill.to_ascii_lowercase();
293300
if matches!(k.as_str(), "off" | "disabled" | "0" | "false" | "halt") {
294301
return Decision::Deny {
@@ -358,10 +365,10 @@ fn normalise_origin(origin: &str) -> String {
358365
// Strip trailing .git
359366
let s = s.strip_suffix(".git").unwrap_or(s);
360367
// git@host:owner/repo → host/owner/repo
361-
if let Some(rest) = s.strip_prefix("git@") {
362-
if let Some((host, path)) = rest.split_once(':') {
363-
return format!("{host}/{path}");
364-
}
368+
if let Some(rest) = s.strip_prefix("git@")
369+
&& let Some((host, path)) = rest.split_once(':')
370+
{
371+
return format!("{host}/{path}");
365372
}
366373
// https://host/path or http://host/path or ssh://host/path
367374
for prefix in ["https://", "http://", "ssh://", "git://"] {
@@ -468,7 +475,7 @@ reason = "upstream homebrew"
468475
"#;
469476

470477
fn registry() -> ExclusionRegistry {
471-
ExclusionRegistry::from_str(FIXTURE).unwrap()
478+
FIXTURE.parse().unwrap()
472479
}
473480

474481
#[test]

shared-context/src/registry_guard.rs

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -137,19 +137,17 @@ fn parse_full_name(url: &str) -> Option<String> {
137137
let s = url.strip_suffix(".git").unwrap_or(url);
138138

139139
// git@host:owner/repo
140-
if let Some(rest) = s.strip_prefix("git@") {
141-
if let Some((_host, path)) = rest.split_once(':') {
142-
return Some(path.to_string());
143-
}
140+
if let Some(rest) = s.strip_prefix("git@")
141+
&& let Some((_host, path)) = rest.split_once(':')
142+
{
143+
return Some(path.to_string());
144144
}
145145

146146
// https://host/owner/repo or ssh://host/owner/repo
147147
for prefix in ["https://", "http://", "ssh://", "git://"] {
148148
if let Some(rest) = s.strip_prefix(prefix) {
149149
// Skip the host segment.
150-
let mut parts = rest.splitn(2, '/');
151-
let _host = parts.next()?;
152-
let path = parts.next()?;
150+
let (_host, path) = rest.split_once('/')?;
153151
return Some(path.to_string());
154152
}
155153
}

shared-context/src/reporting.rs

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
//! Generates reports about fleet execution, findings, and bot performance
55
//! across all registered bots. Supports multiple output formats.
66
7-
use crate::{BotId, Context, Finding, Severity, Tier};
7+
use crate::{Context, Severity, Tier};
88
use chrono::{DateTime, Utc};
99
use serde::{Deserialize, Serialize};
1010
use std::collections::HashMap;
@@ -290,13 +290,13 @@ impl Context {
290290
.unwrap_or_else(|| "-".to_string())
291291
));
292292
}
293-
md.push_str("\n");
293+
md.push('\n');
294294

295295
// Tier Performance
296296
md.push_str("## Tier Performance\n\n");
297297
md.push_str("| Tier | Bots | Completed | Findings | Avg Duration (ms) |\n");
298298
md.push_str("|------|------|-----------|----------|-------------------|\n");
299-
for (_, perf) in &report.tier_performance {
299+
for perf in report.tier_performance.values() {
300300
md.push_str(&format!(
301301
"| {} | {} | {} | {} | {:.0} |\n",
302302
perf.tier, perf.bots_count, perf.completed_count, perf.total_findings, perf.avg_duration_ms
@@ -362,6 +362,7 @@ impl Context {
362362
#[cfg(test)]
363363
mod tests {
364364
use super::*;
365+
use crate::BotId;
365366
use std::path::PathBuf;
366367

367368
#[test]

shared-context/src/storage.rs

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
//! Context storage backends
33
44
use crate::context::Context;
5-
use crate::state::{RepoState, SessionState};
5+
use crate::state::RepoState;
66
use crate::{ContextError, Result};
77
use std::path::{Path, PathBuf};
88
use tracing::{debug, info};
@@ -22,6 +22,8 @@ impl ContextStorage {
2222
}
2323

2424
/// Create storage in default location (~/.gitbot-fleet)
25+
// Fallible (`Result`), so it cannot implement the infallible std `Default` trait; name kept as public API.
26+
#[allow(clippy::should_implement_trait)]
2527
pub fn default() -> Result<Self> {
2628
let home = std::env::var("HOME").map_err(|_| {
2729
ContextError::InvalidState("HOME environment variable not set".to_string())
@@ -86,12 +88,11 @@ impl ContextStorage {
8688
for entry in std::fs::read_dir(&sessions_dir)? {
8789
let entry = entry?;
8890
let path = entry.path();
89-
if path.extension().map(|e| e == "json").unwrap_or(false) {
90-
if let Some(stem) = path.file_stem().and_then(|s| s.to_str()) {
91-
if let Ok(id) = uuid::Uuid::parse_str(stem) {
92-
sessions.push(id);
93-
}
94-
}
91+
if path.extension().map(|e| e == "json").unwrap_or(false)
92+
&& let Some(stem) = path.file_stem().and_then(|s| s.to_str())
93+
&& let Ok(id) = uuid::Uuid::parse_str(stem)
94+
{
95+
sessions.push(id);
9596
}
9697
}
9798

@@ -152,10 +153,10 @@ impl ContextStorage {
152153
for entry in std::fs::read_dir(&repos_dir)? {
153154
let entry = entry?;
154155
let path = entry.path();
155-
if path.extension().map(|e| e == "json").unwrap_or(false) {
156-
if let Some(stem) = path.file_stem().and_then(|s| s.to_str()) {
157-
repos.push(stem.to_string());
158-
}
156+
if path.extension().map(|e| e == "json").unwrap_or(false)
157+
&& let Some(stem) = path.file_stem().and_then(|s| s.to_str())
158+
{
159+
repos.push(stem.to_string());
159160
}
160161
}
161162

@@ -174,17 +175,16 @@ impl ContextStorage {
174175
for entry in std::fs::read_dir(&sessions_dir)? {
175176
let entry = entry?;
176177
let path = entry.path();
177-
if path.extension().map(|e| e == "json").unwrap_or(false) {
178-
if let Ok(metadata) = entry.metadata() {
179-
if let Ok(modified) = metadata.modified() {
180-
sessions.push((path, modified));
181-
}
182-
}
178+
if path.extension().map(|e| e == "json").unwrap_or(false)
179+
&& let Ok(metadata) = entry.metadata()
180+
&& let Ok(modified) = metadata.modified()
181+
{
182+
sessions.push((path, modified));
183183
}
184184
}
185185

186186
// Sort by modification time (newest first)
187-
sessions.sort_by(|a, b| b.1.cmp(&a.1));
187+
sessions.sort_by_key(|b| std::cmp::Reverse(b.1));
188188

189189
// Delete sessions beyond keep limit
190190
let mut deleted = 0;
@@ -223,6 +223,13 @@ pub struct MemoryStorage {
223223
repos: std::collections::HashMap<String, RepoState>,
224224
}
225225

226+
#[cfg(test)]
227+
impl Default for MemoryStorage {
228+
fn default() -> Self {
229+
Self::new()
230+
}
231+
}
232+
226233
#[cfg(test)]
227234
impl MemoryStorage {
228235
pub fn new() -> Self {

0 commit comments

Comments
 (0)