Clean Architecture Infrastructure Refactoring#271
Conversation
- Add OSS Insight API client to infrastructure/http module - Update trending.rs to use new API client - Update trending_unified_view.rs to use new API structure 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Eliminate duplicate git repository root detection across components - Optimize git_root calculation by computing once per batch extraction - Remove duplicate find_git_repository_root implementation from CommonExtractor - Move git-related processing to infrastructure layer following Clean Architecture - Fix cloning_step branching logic and make extraction responsibilities clear - Implement mandatory git_root requirement with proper error handling - Use functional programming patterns (map, and_then) for cleaner code - Add test-specific helper functions to avoid modifying production behavior 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…ture - Move storage/database.rs to database/database.rs - Move storage/daos/ to database/daos/ - Move storage/migrations/ to database/migrations/ - Move storage/seeders/ to database/seeders/ - Update all imports to use new database module structure - Remove storage/integration_test.rs (functionality moved to unit tests) - Clean up storage module to only contain file-based storage if needed This completes the Clean Architecture separation by creating a dedicated database infrastructure module separate from general storage concerns. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Move GitHub API communication to infrastructure/http module - Create generic FileStorage in infrastructure/storage/file without cache-specific terminology - Extract version checking business logic to domain layer (repositories and services) - Repository layer handles cache concerns while Service layer remains cache-agnostic - Delete infrastructure/version directory completely - Add comprehensive tests with mock implementations using cfg(feature) pattern - Tests no longer depend on Internet connectivity or CARGO_PKG_VERSION - Self-referential dev-dependency pattern enables automatic mock activation during testing 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…leStorage - Move ConfigManager from infrastructure/config to domain/services layer - Rename ConfigManager to ConfigService following Clean Architecture principles - Update ConfigService to use infrastructure/storage/file/file_storage.rs for file operations - Remove CLI config option and simplify ThemeManager initialization - Add comprehensive tests for ConfigService in tests/unit/domain/services/ - Remove infrastructure/config module and external module references - Update all imports to use new ConfigService location 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add AppDataProvider trait with default implementation for get_app_data_dir - Consolidate file_storage and compressed_file_storage to use shared implementation - Use conditional compilation for production vs test environments - Remove duplicate code while maintaining feature parity - All 594 tests passing 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…st improvements - Move ChallengeCache to domain/repositories/ChallengeRepository - Move TrendingCache to domain/repositories/TrendingRepository - Implement Repository pattern with unified caching and API integration - Fix flaky tests by removing shared static storage - Add comprehensive test coverage for both repositories - Remove conditional compilation in favor of instance-based storage - Add git_repository_ref_parser comprehensive test suite - Resolve all compilation warnings 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add module_inception allow for database mod.rs and version_service_tests.rs - Fix absurd extreme comparisons in remote_git_repository_client.rs (total <= 0 to total == 0) - Apply cargo fmt for consistent code formatting 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughRestructures infrastructure (adds database/git/http; removes cache/config/version/repo_manager), introduces new domain repositories/services (ChallengeRepository, TrendingRepository, VersionRepository, ConfigService), adds Git parsing/clients, refactors extractors to require git_root, updates CLI/game flows and views to new APIs, and revises tests accordingly. Adds async-trait and feature flags. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI
participant VersionService
participant VersionRepository
participant GitHubApiClient
CLI->>VersionService: check()
VersionService->>VersionRepository: fetch_latest_version()
alt Cache valid
VersionRepository-->>VersionService: latest_version
else Cache miss/invalid
VersionRepository->>GitHubApiClient: fetch_latest_release()
GitHubApiClient-->>VersionRepository: GitHubRelease(tag)
VersionRepository-->>VersionService: latest_version (cached)
end
VersionService-->>CLI: (has_update, current, latest)
sequenceDiagram
autonumber
participant UI
participant TrendingRepository
participant OssInsightClient
participant Storage as CompressedFileStorage
UI->>TrendingRepository: get_trending_repositories(key, lang, period)
TrendingRepository->>Storage: load(cache_file)
alt Cache hit
Storage-->>TrendingRepository: data
TrendingRepository-->>UI: repositories
else Cache miss
TrendingRepository->>OssInsightClient: fetch_trending_repositories(lang, period)
alt API ok
OssInsightClient-->>TrendingRepository: repos
TrendingRepository->>Storage: save(cache_file, data)
TrendingRepository-->>UI: repos
else API error
TrendingRepository-->>UI: []
end
end
sequenceDiagram
autonumber
participant Loader
participant RemoteGit as RemoteGitRepositoryClient
participant LocalGit as LocalGitRepositoryClient
participant Screen
Loader->>RemoteGit: clone_repository(repo_spec, progress_cb)
RemoteGit-->>Loader: local_repo_path
Loader->>LocalGit: extract_git_repository(local_repo_path)
LocalGit-->>Loader: GitRepository
Loader->>Screen: update progress / set context
sequenceDiagram
autonumber
participant Game
participant ChallengeRepo as ChallengeRepository
participant Storage as CompressedFileStorage
Game->>ChallengeRepo: load_challenges_with_progress(repo, reporter?)
alt Cache present and valid
ChallengeRepo->>Storage: load(cache_file)
Storage-->>ChallengeRepo: challenges
ChallengeRepo-->>Game: Some(challenges)
else Missing/invalid
ChallengeRepo-->>Game: None
Game->>ChallengeRepo: save_challenges(repo, generated)
ChallengeRepo->>Storage: save(cache_file, data)
Storage-->>ChallengeRepo: ok
ChallengeRepo-->>Game: ok
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
Ref: #267 |
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (56.05%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #271 +/- ##
==========================================
+ Coverage 21.89% 22.57% +0.67%
==========================================
Files 187 192 +5
Lines 21895 21603 -292
==========================================
+ Hits 4794 4876 +82
+ Misses 17101 16727 -374
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/domain/models/storage/repository.rs (1)
1-27: Preserve clean architecture layering between domain and infrastructureImporting
GitRepositoryRefParserfrominfrastructureinside a domain model ties the core domain layer back to infrastructure, reversing the dependency flow this refactor is trying to enforce. That coupling risks cyclic builds and makes future infrastructure swaps impossible without touching domain code. Please move the parsing logic into the domain (or inject the already-parsed reference from infrastructure) so the domain stays infrastructure-agnostic.src/domain/services/theme_manager.rs (2)
88-93: Major: path handling bypasses AppDataProvider; inconsistent across OS/debug/tests.Using HOME and defaulting to "." conflicts with AppDataProvider (debug uses current_dir, test-mocks uses /tmp/test, Windows may lack HOME). This can split config vs theme locations and break tests or Windows users. Switch to AppDataProvider.
Imports to add (top of file):
+use crate::infrastructure::storage::{file_storage::FileStorage, AppDataProvider};Function change:
- fn get_custom_theme_path() -> std::path::PathBuf { - let home_dir = std::env::var("HOME").unwrap_or_else(|_| ".".to_string()); - std::path::PathBuf::from(home_dir) - .join(".gittype") - .join("custom-theme.json") - } + fn get_custom_theme_path() -> std::path::PathBuf { + // Align with AppDataProvider semantics (debug/test-mocks/OS-specific dirs) + let app_dir = <FileStorage as AppDataProvider>::get_app_data_dir() + .unwrap_or_else(|_| std::env::current_dir().unwrap_or_else(|_| ".".into())); + app_dir.join("custom-theme.json") + }Based on learnings.
88-93: Consolidate all hard-coded “.gittype” paths into AppDataProvider
Multiple modules still construct paths viaenv::var("HOME")ordirs::home_dir()then.join(".gittype")(e.g. in infrastructure/logging.rs, git/remote/remote_git_repository_client.rs, cli/commands/repo.rs, cli/views/repo_list_view.rs, infrastructure/database/database.rs, domain/services/theme_manager.rs, domain/repositories/*, and various presentation/game/screens). Replace these with a single, centralized helper in AppDataProvider (e.g.AppDataProvider::base_dir()?.join("…")) to ensure consistency and simplify future changes.src/domain/repositories/trending_repository.rs (1)
91-134: Avoid blocking std::fs calls inside async path
get_trending_repositoriesisasync, but bothget_from_cacheandsave_to_cachesynchronously callstd::fs::read_to_string,fs::write, andfs::remove_file. On a Tokio runtime this blocks the worker thread and can starve unrelated tasks. Please move the disk I/O onto Tokio’s blocking pool (tokio::fsortokio::task::spawn_blocking) so the async path stays non-blocking.(docs.rs)
🧹 Nitpick comments (36)
src/presentation/game/screens/settings_screen.rs (1)
117-125: Surface config persistence failures.We still drop both the constructor and
save()errors on the floor, so a filesystem issue will quietly lose settings. Even alog::warn!on theErrpaths would help diagnose user reports.src/infrastructure/git/mod.rs (1)
1-7: Public API consolidation looks good; consider minimal docs.Re-exports create a clean surface. Consider adding a short module-level doc and brief item docs to make the intended entry points clear to downstream users.
src/domain/repositories/mod.rs (1)
1-6: Consider limiting module visibility to reduce public surface.If external crates shouldn’t rely on module paths, make new modules private and keep the public re-exports. This preserves the API of traits/types without exposing layout details.
Example change:
-pub mod challenge_repository; +mod challenge_repository; ... -pub mod trending_repository; +mod trending_repository; -pub mod version_repository; +mod version_repository;Can you confirm whether these modules are intentionally part of the public API? If yes, ignore this suggestion.
src/domain/services/theme_manager.rs (4)
21-29: Minor: rename local for clarity.Variable name suggests the old type. Use
config_serviceto match the new type.- pub fn init() -> anyhow::Result<()> { - let config_manager = ConfigService::new()?; + pub fn init() -> anyhow::Result<()> { + let config_service = ConfigService::new()?; ... - let config = config_manager.get_config(); + let config = config_service.get_config();
32-32: Prefer expect over unwrap on lock to aid diagnostics.Panic message will be clearer if the lock is poisoned.
- let mut manager = THEME_MANAGER.write().unwrap(); + let mut manager = THEME_MANAGER + .write() + .expect("THEME_MANAGER write lock poisoned"); - let manager = THEME_MANAGER.read().unwrap(); + let manager = THEME_MANAGER + .read() + .expect("THEME_MANAGER read lock poisoned");Also applies to: 47-49
61-85: Tiny: avoid recomputing the theme path.Store
let custom_path = Self::get_custom_theme_path();and reuse it forexists(),read_to_stringto reduce duplicated I/O and path computation.
95-121: Optional: use shared storage helpers for JSON I/O.Consider reading/writing the custom theme via your FileStorage to centralize serialization, error handling, and testing (mocks).
src/domain/services/config_manager.rs (1)
29-36: Consider atomic writes and recovery from malformed JSON.
- Write config to a temp file and rename to avoid partial writes on crash.
- Optionally, on parse errors, back up the old file and start with defaults to keep the app usable.
Can you confirm whether
FileStorage.write_jsonis atomic and whetherread_jsondistinguishes “missing” vs “malformed”? If not, I can propose a smallwrite_json_atomichelper and error-handling pattern.src/infrastructure/storage/mod.rs (1)
1-6: Storage surface looks cohesive.Splitting storage concerns and re-exporting AppDataProvider aligns with the refactor goals. Consider a brief module doc explaining intended consumers for each submodule.
src/infrastructure/storage/app_data_provider.rs (2)
9-21: Make debug and release resolve a consistent app subdirectory.Writing directly to the CWD in debug is surprising; append “.gittype” in both modes to avoid polluting project roots.
- let data_dir = if cfg!(debug_assertions) { - std::env::current_dir().map_err(|e| { + let base_dir = if cfg!(debug_assertions) { + std::env::current_dir().map_err(|e| { GitTypeError::ExtractionFailed(format!( "Could not get current directory: {}", e )) })? } else { let home_dir = dirs::home_dir().ok_or_else(|| { GitTypeError::ExtractionFailed("Could not determine home directory".to_string()) })?; - home_dir.join(".gittype") + home_dir }; + let data_dir = base_dir.join(".gittype");
17-21: Consider OS‑appropriate data dir (XDG/Windows/macOS).
dirs::home_dir().join(".gittype")is simple but non‑standard. Preferdirs::data_dir()or a higher‑level crate likedirectoriesfor platform‑correct paths (e.g.,%APPDATA%\\gittype,~/Library/Application Support/gittype,$XDG_DATA_HOME/gittype).tests/unit/domain/services/version_service_tests.rs (1)
1-2: Avoid module‑inception; flatten tests.The inner
mod version_service_teststriggers clippy and needs an allow. Prefer top‑level tests in this file and drop the module andallow. Keeps clippy clean.src/infrastructure/git/local/local_git_repository_client.rs (4)
15-29: UseRepository::discoverfor robust root detection (nested dirs, worktrees, bare repos).Walking for
.gitmisses bare repositories and can mis-handle worktrees/symlinks. Let libgit2 resolve it, then derive the root.- pub fn get_repository_root(path: &Path) -> Option<PathBuf> { - let mut current_path = path.to_path_buf(); - - loop { - if Self::is_git_repository(¤t_path) { - return Some(current_path); - } - - if !current_path.pop() { - break; - } - } - - None - } + pub fn get_repository_root(path: &Path) -> Option<PathBuf> { + Repository::discover(path) + .ok() + .and_then(|repo| { + repo.workdir() + .map(|p| p.to_path_buf()) + .or_else(|| Some(repo.path().to_path_buf())) + }) + } @@ - let canonical_path = repo_path.canonicalize().map_err(|_| { - GitTypeError::ExtractionFailed("Path canonicalization failed".to_string()) - })?; - - let git_root = Self::get_repository_root(&canonical_path).ok_or_else(|| { - GitTypeError::ExtractionFailed("Git repository not found".to_string()) - })?; - - let repo = Repository::open(&git_root).map_err(|e| { - GitTypeError::ExtractionFailed(format!("Failed to open git repository: {}", e)) - })?; + let canonical_path = repo_path.canonicalize().map_err(|e| { + GitTypeError::ExtractionFailed(format!( + "Path canonicalization failed for {}: {}", + repo_path.display(), + e + )) + })?; + + let repo = Repository::discover(&canonical_path).map_err(|e| { + GitTypeError::ExtractionFailed(format!( + "Failed to discover git repository from {}: {}", + canonical_path.display(), + e + )) + })?; + + let git_root = repo + .workdir() + .map(|p| p.to_path_buf()) + .unwrap_or_else(|| repo.path().to_path_buf());Also applies to: 31-43
77-86: Clarify branch error for detached HEAD.The current message blames UTF‑8; detached HEAD is a common, different case. Return a better message.
- .and_then(|name_opt| { - name_opt.ok_or_else(|| { - GitTypeError::ExtractionFailed("Branch name is not valid UTF-8".to_string()) - }) - }) + .and_then(|name_opt| { + name_opt.ok_or_else(|| { + GitTypeError::ExtractionFailed( + "HEAD is detached or branch name is not Unicode".to_string(), + ) + }) + })
88-97: Resolve commit viapeel_to_commitfor both symbolic and detached HEAD.
head.target()may beNonefor symbolic refs. Peeling is more robust.- fn get_current_commit_hash(repo: &Repository) -> Result<String> { - repo.head() - .map_err(|e| GitTypeError::ExtractionFailed(format!("Failed to get HEAD: {}", e))) - .map(|head| head.target().map(|oid| oid.to_string())) - .and_then(|oid_opt| { - oid_opt.ok_or_else(|| { - GitTypeError::ExtractionFailed("HEAD does not point to a commit".to_string()) - }) - }) - } + fn get_current_commit_hash(repo: &Repository) -> Result<String> { + let commit = repo + .head() + .and_then(|h| h.peel_to_commit()) + .map_err(|e| { + GitTypeError::ExtractionFailed(format!("Failed to resolve HEAD to commit: {}", e)) + })?; + Ok(commit.id().to_string()) + }
31-62: Propagate parse errors verbosely.Swallowing the parser’s error obscures root causes (e.g., unsupported protocols). Include details.
- let repo_ref = GitRepositoryRefParser::parse(&remote_url).map_err(|_| { - GitTypeError::ExtractionFailed("Failed to parse remote URL".to_string()) - })?; + let repo_ref = GitRepositoryRefParser::parse(&remote_url).map_err(|e| { + GitTypeError::ExtractionFailed(format!("Failed to parse remote URL '{}': {}", remote_url, e)) + })?;src/infrastructure/http/oss_insight_client.rs (4)
32-40: Build URL with a proper builder to avoid encoding/concat pitfallsUse
reqwest::Url(re-export of url) to assemble query params. This removes manual concat and double‑encoding risks.- let mut url = format!( - "https://api.ossinsight.io/v1/trends/repos/?period={}", - api_period - ); - - if let Some(lang) = language { - let api_lang = self.map_language_name(lang); - url = format!("{}&language={}", url, urlencoding::encode(&api_lang)); - } + let mut url = reqwest::Url::parse("https://api.ossinsight.io/v1/trends/repos/") + .expect("static URL"); + url.query_pairs_mut().append_pair("period", api_period); + if let Some(lang) = language { + let api_lang = self.map_language_name(lang); + url.query_pairs_mut().append_pair("language", &api_lang); + }
51-56: Include response body snippet on non‑success to aid diagnosisSurface a short body excerpt alongside status. Helps when API returns structured errors.
- if !response.status().is_success() { - return Err(GitTypeError::ApiError(format!( - "OSS Insight API request failed: {}", - response.status() - ))); - } + if !response.status().is_success() { + let status = response.status(); + let body = response.text().await.unwrap_or_default(); + let preview = body.chars().take(200).collect::<String>(); + return Err(GitTypeError::ApiError(format!( + "OSS Insight API request failed: {} — {}", + status, preview + ))); + }
64-81: Title‑case hyphenated or space‑separated languages (Objective‑C, Visual Basic)Current logic lowercases the rest of the string, breaking
Objective-C. Title‑case by segments while preserving known special cases.fn map_language_name(&self, lang: &str) -> String { match lang.to_lowercase().as_str() { "javascript" => "JavaScript".to_string(), "typescript" => "TypeScript".to_string(), "c++" => "C++".to_string(), "c#" => "C#".to_string(), "php" => "PHP".to_string(), _ => { - let mut chars = lang.chars(); - match chars.next() { - None => lang.to_string(), - Some(first) => { - first.to_uppercase().collect::<String>() + &chars.as_str().to_lowercase() - } - } + // Title-case by '-' and ' ' while leaving other punctuation intact + lang.split(&['-', ' '][..]) + .map(|seg| { + let mut it = seg.chars(); + match it.next() { + None => String::new(), + Some(f) => f.to_uppercase().collect::<String>() + + &it.as_str().to_lowercase() + } + }) + .collect::<Vec<_>>() + .join(if lang.contains('-') { "-" } else { " " }) } } }
20-31: Validate period early or document fallbackUnknown periods silently map to daily. Either validate here with a
ValidationErroror document the fallback; consider aligning with CLI validation.src/presentation/cli/commands/trending.rs (6)
41-52: Language validation UX is good; consider sharing the source of truthValidation duplicates language knowledge that also exists in the HTTP client’s mapping. Extract a shared normalizer/validator to avoid drift.
55-56: Remove the dummy_client: &()parameterThis arg is unused and leaks refactor scaffolding into the public surface.
- let repos = fetch_trending_repositories_cached(&(), language.as_deref(), &period).await?; + let repos = fetch_trending_repositories_cached(language.as_deref(), &period).await?;And change the function signature below accordingly.
Also applies to: 72-73
112-121: Clean upfetch_trending_repositories_cachedsignature and normalize cache keyNormalize language for the key to prevent duplicate entries (
"Rust"vs"rust").-pub async fn fetch_trending_repositories_cached( - _client: &(), - language: Option<&str>, - period: &str, -) -> Result<Vec<TrendingRepositoryInfo>> { - let cache_key = format!("{}:{}", language.unwrap_or("all"), period); +pub async fn fetch_trending_repositories_cached( + language: Option<&str>, + period: &str, +) -> Result<Vec<TrendingRepositoryInfo>> { + let cache_key = format!( + "{}:{}", + language.map(|s| s.to_ascii_lowercase()).as_deref().unwrap_or("all"), + period + );
119-132: Error handling: consider surfacing a concise user hint instead of silently empty listSwallowing errors degrades silently. At least print a one‑liner to stderr for CLI users while keeping graceful fallback.
- Err(e) => { - log::warn!("Failed to retrieve trending repositories: {}", e); - Ok(Vec::new()) // Return empty vec for graceful degradation - } + Err(e) => { + log::warn!("Failed to retrieve trending repositories: {}", e); + eprintln!("⚠️ Unable to fetch trending repositories right now. Showing no results."); + Ok(Vec::new()) + }
135-142: Repository selection: prefer exact match before substringAvoid surprising matches when many repos contain the same keyword. Try exact owner/repo first, then fallback to substring.
fn select_repository_by_name<'a>( repos: &'a [TrendingRepositoryInfo], name: &str, ) -> Option<&'a TrendingRepositoryInfo> { - repos - .iter() - .find(|repo| repo.repo_name.to_lowercase().contains(&name.to_lowercase())) + let needle = name.to_ascii_lowercase(); + repos.iter().find(|r| r.repo_name.eq_ignore_ascii_case(name)) + .or_else(|| { + repos.iter().find(|r| { + r.repo_name + .rsplit('/') + .next() + .map(|base| base.eq_ignore_ascii_case(&needle)) + .unwrap_or(false) + }) + }) + .or_else(|| { + repos + .iter() + .find(|r| r.repo_name.to_ascii_lowercase().contains(&needle)) + }) }
35-39: Validateperiodinput at the CLI boundaryProvide fast feedback for typos and align with server expectations (
daily|weekly|monthly).pub async fn run_trending( language: Option<String>, repo_name: Option<String>, period: String, ) -> Result<()> { + const SUPPORTED_PERIODS: &[&str] = &["daily", "weekly", "monthly"]; + if !SUPPORTED_PERIODS.contains(&period.as_str()) { + eprintln!("❌ Unsupported period: '{}'", period); + eprintln!("📚 Supported periods: daily, weekly, monthly"); + return Err(GitTypeError::ValidationError(format!( + "Unsupported period: {}", + period + ))); + }tests/unit/domain/repositories/trending_repository_tests.rs (4)
6-8: Avoid hardcoded cache path; use temp dir to keep tests hermetic.
Hardcoding “/mock/trending_cache” can fail on CI or non-Unix hosts. Prefer OS temp dir.-fn create_test_trending_repository() -> TrendingRepository { - TrendingRepository::with_cache_dir(PathBuf::from("/mock/trending_cache")) -} +fn create_test_trending_repository() -> TrendingRepository { + let dir = std::env::temp_dir().join("gittype_trending_cache_test"); + TrendingRepository::with_cache_dir(dir) +}
25-35: Unit tests should not hit the live network.
These calls exercise real HTTP; results are nondeterministic and rate‑limit prone. Gate tests behind a mock (feature = "test-mocks") or inject a mock client into TrendingRepository.If a mock feature exists (as with GitHubApiClient), consider mirroring it for OssInsightClient and running these tests with
--features test-mocks.
63-74: Brittle assumption about failure path.
Asserting Ok+empty on “invalid” inputs ties the test to current error‑handling. Prefer explicit mocking to force an error and verify graceful degradation in a controlled way.
99-109: Implement the Default trait for TrendingRepository.
CallingTrendingRepository::default()works only because there’s an inherentfn default(). Idiomatic Rust expectsimpl Default. Consider implementing the trait and keeping the inherent method delegating to it.src/infrastructure/http/github_api_client.rs (2)
33-51: Harden the request: GitHub headers, optional auth, better error context.
Add Accept and API‑Version headers, supportGITHUB_TOKENfor higher rate limits, and include a short body snippet on non‑2xx for diagnostics.- pub async fn fetch_latest_release(&self) -> Result<GitHubRelease> { - let url = "https://api.github.com/repos/unhappychoice/gittype/releases/latest"; - let response = self.client.get(url).send().await.map_err(|e| { + pub async fn fetch_latest_release(&self) -> Result<GitHubRelease> { + let url = "https://api.github.com/repos/unhappychoice/gittype/releases/latest"; + let mut req = self + .client + .get(url) + .header("Accept", "application/vnd.github+json") + .header("X-GitHub-Api-Version", "2022-11-28"); + if let Ok(token) = std::env::var("GITHUB_TOKEN") { + req = req.bearer_auth(token); + } + let response = req.send().await.map_err(|e| { GitTypeError::ExtractionFailed(format!("Failed to fetch release: {}", e)) })?; if !response.status().is_success() { - return Err(GitTypeError::ExtractionFailed(format!( - "GitHub API request failed with status: {}", - response.status() - ))); + let status = response.status(); + let snippet = response.text().await.unwrap_or_default(); + let snippet = snippet.chars().take(200).collect::<String>(); + return Err(GitTypeError::ExtractionFailed(format!( + "GitHub API request failed: {} - {}", + status, snippet + ))); } let release: GitHubRelease = response.json().await.map_err(|e| { GitTypeError::ExtractionFailed(format!("Failed to parse JSON: {}", e)) })?; Ok(release) }
20-31: Consider making owner/repo configurable.
Hardcoding the repository ties the client to a single project. Accept(owner, repo)innew()orfetch_latest_release()and default to the current repo when not provided.src/domain/services/version_service.rs (1)
53-63: Prefer the semver crate for robustness.
If tags include pre‑releases (e.g., “1.2.0‑beta”), numeric parsing will fail. Usingsemver::Versionwould handle this cleanly.src/domain/repositories/version_repository.rs (2)
54-56: Avoid disabling cache in debug builds globally.
ReturningNonein debug bypasses cache for all debug runs, including integration tests, causing unnecessary network calls. Gate this under a test feature (e.g.,test-mocks) or an env flag.- if cfg!(debug_assertions) { + #[cfg(feature = "test-mocks")] + { return Ok(None); - } + }
104-107: Normalize tags case‑insensitively and trim whitespace.
Git tags sometimes use uppercase ‘V’ or have stray whitespace.- fn normalize_version_tag(tag: &str) -> String { - tag.strip_prefix('v').unwrap_or(tag).to_string() - } + fn normalize_version_tag(tag: &str) -> String { + let t = tag.trim(); + if let Some(rest) = t.strip_prefix('v').or_else(|| t.strip_prefix('V')) { + rest.to_string() + } else { + t.to_string() + } + }src/infrastructure/git/remote/remote_git_repository_client.rs (1)
46-48: Be cautious removing existing directories.
Blindremove_dir_allcan delete user data if path resolution misbehaves. Consider validating the path prefix (e.g., ensure it’s under$HOME/.gittype/repos) before removal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (85)
Cargo.toml(3 hunks)examples/seed_database.rs(1 hunks)src/domain/models/git_repository_ref.rs(1 hunks)src/domain/models/mod.rs(2 hunks)src/domain/models/storage/repository.rs(2 hunks)src/domain/repositories/challenge_repository.rs(6 hunks)src/domain/repositories/git_repository_repository.rs(1 hunks)src/domain/repositories/mod.rs(1 hunks)src/domain/repositories/session_repository.rs(1 hunks)src/domain/repositories/stage_repository.rs(1 hunks)src/domain/repositories/trending_repository.rs(7 hunks)src/domain/repositories/version_repository.rs(1 hunks)src/domain/services/config_manager.rs(1 hunks)src/domain/services/extractor/code_chunk_extractor.rs(5 hunks)src/domain/services/extractor/core/extractor.rs(2 hunks)src/domain/services/extractor/git_repository_extractor.rs(0 hunks)src/domain/services/extractor/mod.rs(0 hunks)src/domain/services/mod.rs(1 hunks)src/domain/services/theme_manager.rs(2 hunks)src/domain/services/version_service.rs(1 hunks)src/infrastructure/cache/gzip_storage.rs(0 hunks)src/infrastructure/cache/mod.rs(0 hunks)src/infrastructure/config/mod.rs(0 hunks)src/infrastructure/database/mod.rs(1 hunks)src/infrastructure/database/seeders/database_seeder.rs(1 hunks)src/infrastructure/external/mod.rs(0 hunks)src/infrastructure/git/git_repository_ref_parser.rs(1 hunks)src/infrastructure/git/local/local_git_repository_client.rs(1 hunks)src/infrastructure/git/local/mod.rs(1 hunks)src/infrastructure/git/mod.rs(1 hunks)src/infrastructure/git/remote/mod.rs(1 hunks)src/infrastructure/git/remote/remote_git_repository_client.rs(1 hunks)src/infrastructure/http/github_api_client.rs(1 hunks)src/infrastructure/http/mod.rs(1 hunks)src/infrastructure/http/oss_insight_client.rs(1 hunks)src/infrastructure/mod.rs(1 hunks)src/infrastructure/repository_manager.rs(0 hunks)src/infrastructure/storage/app_data_provider.rs(1 hunks)src/infrastructure/storage/compressed_file_storage.rs(1 hunks)src/infrastructure/storage/file_storage.rs(1 hunks)src/infrastructure/storage/integration_test.rs(0 hunks)src/infrastructure/storage/mod.rs(1 hunks)src/infrastructure/version/cache.rs(0 hunks)src/infrastructure/version/checker.rs(0 hunks)src/infrastructure/version/mod.rs(0 hunks)src/presentation/cli/args.rs(0 hunks)src/presentation/cli/commands/game.rs(4 hunks)src/presentation/cli/commands/repo.rs(1 hunks)src/presentation/cli/commands/trending.rs(4 hunks)src/presentation/cli/runner.rs(3 hunks)src/presentation/cli/views/mod.rs(0 hunks)src/presentation/cli/views/repo_list_view.rs(3 hunks)src/presentation/cli/views/repo_play_view.rs(2 hunks)src/presentation/cli/views/repo_utils.rs(0 hunks)src/presentation/cli/views/trending_repository_selection_view.rs(2 hunks)src/presentation/cli/views/trending_unified_view.rs(3 hunks)src/presentation/game/models/loading_steps/cache_check_step.rs(2 hunks)src/presentation/game/models/loading_steps/cloning_step.rs(2 hunks)src/presentation/game/models/loading_steps/database_init_step.rs(1 hunks)src/presentation/game/models/loading_steps/generating_step.rs(2 hunks)src/presentation/game/models/loading_steps/scanning_step.rs(2 hunks)src/presentation/game/screens/records_screen.rs(1 hunks)src/presentation/game/screens/settings_screen.rs(2 hunks)tests/integration/comment_processing_tests.rs(2 hunks)tests/integration/indent_treesitter_tests.rs(1 hunks)tests/integration/mod.rs(3 hunks)tests/unit/domain/mod.rs(1 hunks)tests/unit/domain/repositories/challenge_repository_tests.rs(1 hunks)tests/unit/domain/repositories/mod.rs(1 hunks)tests/unit/domain/repositories/trending_repository_tests.rs(1 hunks)tests/unit/domain/services/config_service_tests.rs(1 hunks)tests/unit/domain/services/extractor/core/mod.rs(1 hunks)tests/unit/domain/services/extractor/mod.rs(1 hunks)tests/unit/domain/services/mod.rs(1 hunks)tests/unit/domain/services/version_service_tests.rs(1 hunks)tests/unit/infrastructure/cache/challenge_cache_tests.rs(0 hunks)tests/unit/infrastructure/cache/mod.rs(0 hunks)tests/unit/infrastructure/git/git_repository_ref_parser_test.rs(1 hunks)tests/unit/infrastructure/git/local_git_repository_client_test.rs(1 hunks)tests/unit/infrastructure/git/mod.rs(1 hunks)tests/unit/infrastructure/git/remote_git_repository_client_test.rs(1 hunks)tests/unit/infrastructure/mod.rs(1 hunks)tests/unit/infrastructure/version/cache_test.rs(0 hunks)tests/unit/infrastructure/version/checker_test.rs(0 hunks)tests/unit/infrastructure/version/mod.rs(0 hunks)
💤 Files with no reviewable changes (19)
- src/infrastructure/version/mod.rs
- tests/unit/infrastructure/cache/mod.rs
- tests/unit/infrastructure/version/mod.rs
- src/infrastructure/config/mod.rs
- tests/unit/infrastructure/version/checker_test.rs
- src/infrastructure/version/checker.rs
- src/domain/services/extractor/mod.rs
- src/presentation/cli/args.rs
- src/presentation/cli/views/mod.rs
- src/presentation/cli/views/repo_utils.rs
- tests/unit/infrastructure/version/cache_test.rs
- src/infrastructure/repository_manager.rs
- src/infrastructure/external/mod.rs
- src/infrastructure/storage/integration_test.rs
- src/domain/services/extractor/git_repository_extractor.rs
- src/infrastructure/cache/mod.rs
- src/infrastructure/cache/gzip_storage.rs
- src/infrastructure/version/cache.rs
- tests/unit/infrastructure/cache/challenge_cache_tests.rs
🧰 Additional context used
🧬 Code graph analysis (43)
tests/unit/domain/services/config_service_tests.rs (1)
src/domain/services/config_manager.rs (2)
new(11-19)get_config(21-23)
src/domain/models/git_repository_ref.rs (1)
src/domain/models/storage/repository.rs (1)
http_url(23-27)
tests/unit/infrastructure/git/local_git_repository_client_test.rs (1)
src/infrastructure/git/local/local_git_repository_client.rs (1)
is_git_repository(10-13)
src/presentation/cli/views/repo_play_view.rs (1)
src/infrastructure/git/remote/remote_git_repository_client.rs (1)
is_repository_cached(99-105)
src/presentation/game/models/loading_steps/cloning_step.rs (4)
src/infrastructure/git/remote/remote_git_repository_client.rs (1)
clone_repository(32-90)src/infrastructure/git/local/local_git_repository_client.rs (1)
extract_git_repository(31-62)src/presentation/game/screens/loading_screen.rs (1)
set_git_repository(187-217)src/presentation/game/session_manager.rs (1)
set_git_repository(244-254)
src/domain/services/theme_manager.rs (1)
src/domain/services/config_manager.rs (1)
new(11-19)
src/infrastructure/database/seeders/database_seeder.rs (2)
src/domain/repositories/git_repository_repository.rs (1)
database(69-71)src/infrastructure/database/database.rs (1)
database(118-118)
tests/unit/domain/services/version_service_tests.rs (2)
src/domain/repositories/version_repository.rs (1)
new(16-21)src/domain/services/version_service.rs (1)
new(9-13)
tests/unit/domain/repositories/trending_repository_tests.rs (2)
src/domain/repositories/trending_repository.rs (3)
with_cache_dir(53-61)default(33-35)new(39-51)src/infrastructure/http/oss_insight_client.rs (2)
default(101-103)new(14-18)
src/presentation/game/models/loading_steps/database_init_step.rs (5)
src/domain/repositories/git_repository_repository.rs (1)
database(69-71)src/domain/repositories/session_repository.rs (1)
database(478-480)src/domain/repositories/stage_repository.rs (1)
database(86-88)src/infrastructure/database/seeders/database_seeder.rs (1)
database(261-263)src/infrastructure/database/database.rs (1)
database(118-118)
src/infrastructure/http/oss_insight_client.rs (1)
src/domain/repositories/trending_repository.rs (1)
new(39-51)
src/infrastructure/database/mod.rs (5)
src/domain/repositories/git_repository_repository.rs (1)
database(69-71)src/domain/repositories/session_repository.rs (1)
database(478-480)src/domain/repositories/stage_repository.rs (1)
database(86-88)src/infrastructure/database/seeders/database_seeder.rs (1)
database(261-263)src/infrastructure/database/database.rs (1)
database(118-118)
src/presentation/game/screens/records_screen.rs (2)
src/domain/repositories/session_repository.rs (1)
database(478-480)src/infrastructure/database/database.rs (1)
database(118-118)
src/infrastructure/git/remote/remote_git_repository_client.rs (1)
src/infrastructure/git/git_repository_ref_parser.rs (1)
parse(7-17)
src/domain/repositories/session_repository.rs (4)
src/domain/repositories/git_repository_repository.rs (1)
database(69-71)src/domain/repositories/stage_repository.rs (1)
database(86-88)src/infrastructure/database/seeders/database_seeder.rs (1)
database(261-263)src/infrastructure/database/database.rs (1)
database(118-118)
src/presentation/cli/commands/trending.rs (1)
src/domain/repositories/trending_repository.rs (1)
new(39-51)
src/domain/repositories/stage_repository.rs (4)
src/domain/repositories/git_repository_repository.rs (1)
database(69-71)src/domain/repositories/session_repository.rs (1)
database(478-480)src/infrastructure/database/seeders/database_seeder.rs (1)
database(261-263)src/infrastructure/database/database.rs (1)
database(118-118)
src/domain/repositories/version_repository.rs (4)
src/domain/services/config_manager.rs (2)
FileStorage(14-15)new(11-19)src/domain/services/version_service.rs (1)
new(9-13)src/infrastructure/http/github_api_client.rs (2)
new(21-31)new(62-64)src/infrastructure/storage/app_data_provider.rs (1)
get_app_data_dir(5-31)
src/domain/services/extractor/code_chunk_extractor.rs (3)
src/domain/services/extractor/parsers/mod.rs (1)
parse_with_thread_local(179-197)src/infrastructure/git/local/local_git_repository_client.rs (1)
get_repository_root(15-29)src/domain/services/extractor/core/extractor.rs (1)
extract_chunks_from_tree(11-194)
tests/unit/domain/repositories/challenge_repository_tests.rs (1)
src/domain/repositories/challenge_repository.rs (4)
with_cache_dir(47-52)save_challenges(54-92)clear_cache(147-153)invalidate_repository(165-175)
src/domain/models/storage/repository.rs (2)
src/domain/models/git_repository_ref.rs (1)
http_url(9-11)src/infrastructure/git/git_repository_ref_parser.rs (1)
parse(7-17)
src/presentation/game/screens/settings_screen.rs (1)
src/domain/services/config_manager.rs (1)
new(11-19)
src/domain/repositories/trending_repository.rs (2)
src/domain/repositories/challenge_repository.rs (3)
default(279-281)new(36-45)with_cache_dir(47-52)src/infrastructure/http/oss_insight_client.rs (2)
default(101-103)new(14-18)
src/infrastructure/git/local/local_git_repository_client.rs (1)
src/infrastructure/git/git_repository_ref_parser.rs (1)
parse(7-17)
tests/integration/mod.rs (3)
src/domain/services/extractor/parsers/mod.rs (2)
parse_with_thread_local(179-197)new(52-87)src/domain/services/extractor/core/extractor.rs (1)
extract_chunks_from_tree(11-194)src/domain/services/extractor/language_registry.rs (1)
from_extension(65-69)
tests/integration/comment_processing_tests.rs (1)
tests/integration/mod.rs (1)
extract_from_file_for_test(50-59)
src/presentation/cli/views/trending_unified_view.rs (2)
src/domain/repositories/trending_repository.rs (1)
new(39-51)src/infrastructure/http/oss_insight_client.rs (1)
new(14-18)
src/presentation/cli/views/repo_list_view.rs (1)
src/infrastructure/git/remote/remote_git_repository_client.rs (1)
is_repository_cached(99-105)
src/infrastructure/storage/file_storage.rs (2)
src/domain/services/config_manager.rs (1)
FileStorage(14-15)src/infrastructure/storage/compressed_file_storage.rs (4)
delete_file(77-82)delete_file(182-186)file_exists(104-106)file_exists(188-191)
src/domain/services/extractor/core/extractor.rs (1)
src/domain/services/extractor/parsers/mod.rs (1)
get_parser_registry(170-172)
src/domain/repositories/challenge_repository.rs (2)
src/domain/repositories/trending_repository.rs (3)
new(39-51)default(33-35)with_cache_dir(53-61)src/infrastructure/storage/compressed_file_storage.rs (1)
default(123-127)
src/domain/services/version_service.rs (2)
src/domain/repositories/version_repository.rs (1)
new(16-21)src/infrastructure/http/github_api_client.rs (2)
new(21-31)new(62-64)
examples/seed_database.rs (5)
src/domain/repositories/git_repository_repository.rs (1)
database(69-71)src/domain/repositories/session_repository.rs (1)
database(478-480)src/domain/repositories/stage_repository.rs (1)
database(86-88)src/infrastructure/database/seeders/database_seeder.rs (1)
database(261-263)src/infrastructure/database/database.rs (1)
database(118-118)
src/domain/services/config_manager.rs (2)
src/domain/repositories/version_repository.rs (1)
new(16-21)src/infrastructure/storage/app_data_provider.rs (1)
get_app_data_dir(5-31)
tests/unit/infrastructure/git/git_repository_ref_parser_test.rs (1)
src/infrastructure/git/git_repository_ref_parser.rs (1)
parse(7-17)
src/infrastructure/http/github_api_client.rs (1)
src/domain/repositories/version_repository.rs (1)
new(16-21)
src/domain/repositories/git_repository_repository.rs (4)
src/domain/repositories/session_repository.rs (1)
database(478-480)src/domain/repositories/stage_repository.rs (1)
database(86-88)src/infrastructure/database/seeders/database_seeder.rs (1)
database(261-263)src/infrastructure/database/database.rs (1)
database(118-118)
tests/unit/infrastructure/git/remote_git_repository_client_test.rs (1)
src/infrastructure/git/remote/remote_git_repository_client.rs (1)
is_repository_complete(92-97)
tests/integration/indent_treesitter_tests.rs (1)
src/domain/services/extractor/core/extractor.rs (1)
extract_chunks_from_tree(11-194)
src/presentation/cli/commands/game.rs (4)
src/domain/services/version_service.rs (1)
new(9-13)src/presentation/game/screens/version_check_screen.rs (1)
show_legacy(31-44)src/domain/services/theme_manager.rs (1)
init(22-44)src/infrastructure/logging.rs (1)
log_error_to_file(133-163)
src/presentation/cli/commands/repo.rs (2)
src/domain/repositories/git_repository_repository.rs (1)
database(69-71)src/infrastructure/database/database.rs (1)
database(118-118)
src/infrastructure/storage/compressed_file_storage.rs (2)
src/domain/repositories/challenge_repository.rs (3)
new(36-45)default(279-281)storage(184-185)src/infrastructure/storage/file_storage.rs (4)
delete_file(60-65)delete_file(99-101)file_exists(68-70)file_exists(103-105)
src/infrastructure/mod.rs (5)
src/domain/repositories/git_repository_repository.rs (1)
database(69-71)src/domain/repositories/session_repository.rs (1)
database(478-480)src/domain/repositories/stage_repository.rs (1)
database(86-88)src/infrastructure/database/seeders/database_seeder.rs (1)
database(261-263)src/infrastructure/database/database.rs (1)
database(118-118)
🔇 Additional comments (13)
tests/unit/domain/services/config_service_tests.rs (1)
6-36: Confirm tests run against an isolated config pathLine [6] and the other tests invoke
ConfigService::new()/save()directly. Unless the test build swaps in an in-memoryFileStoragevia the new infrastructure, this will hit the real config path (e.g.,~/.config/gittype) and create/overwrite user files when the suite runs. Please confirm that thetest-mockswiring actually shields the filesystem here, or redirect the service to a temp directory inside the test to keep the suite hermetic.src/infrastructure/database/mod.rs (1)
1-5: Clean module façadeThe module surface now mirrors the new database layout and keeps lint noise out—looks good.
src/domain/models/git_repository_ref.rs (1)
1-11: Solid Git repository ref helperDeriving
Debug/Cloneand centralizinghttp_url()generation gives downstream code a clean, testable primitive. Nice addition.src/infrastructure/http/mod.rs (1)
1-5: HTTP façade reads wellRe-exporting the clients from the module root makes the new HTTP layer easy to consume—no issues spotted.
src/presentation/game/models/loading_steps/cloning_step.rs (1)
2-72: Cloning flow integrates smoothlyThe step now wires progress reporting, cloning, and metadata extraction through the new clients without regressions—LGTM.
src/infrastructure/mod.rs (1)
1-3: Module surface matches new infrastructure split.The updated re-exports cleanly mirror the new database/git/http folders without leaving stale modules behind. 👍
Cargo.toml (1)
76-89: Thank you for wiring the test-mocks feature path.Adding
async-traitand the self-referencing dev dependency keeps integration tests aligned with the new mocks while leaving the release feature surface empty, which is exactly what we want here.src/presentation/game/models/loading_steps/cache_check_step.rs (1)
82-92: Repository swap keeps cache-check semantics intact.
load_challenges_with_progressslots into the existing control flow, preserves the early-return on miss, and continues to update progress when available—looks solid.src/presentation/cli/commands/repo.rs (1)
1-8: Imports now reflect the database refactor.Pointing the CLI commands at
infrastructure::databaseand trimming the old config dependency lines everything up with the new layout—nice catch.src/domain/repositories/mod.rs (1)
8-13: Re-exports are consistent and clear.The chosen names and exports align with Clean Architecture boundaries and keep call sites tidy.
src/domain/services/theme_manager.rs (1)
21-44: Sanity check: corrupted config should not brick theme init.Currently, any error in
ConfigService::new()aborts init. If you prefer resilience, catch and log, then proceed with defaults.Would you like a small patch to default on config read errors and log a warning instead?
src/domain/services/config_manager.rs (1)
10-19: Config loading path is clean and aligns with storage provider.Constructing the path via AppDataProvider and defaulting when missing keeps things predictable.
tests/unit/domain/services/version_service_tests.rs (1)
2-2: Remove redundant#[cfg(feature = "test-mocks")]guard
Integration tests already enable thetest-mocksfeature via the crate’s[dev-dependencies], so gating theversion_service_testsmodule is unnecessary.Likely an incorrect or invalid review comment.
| /// Check if a cache entry is still valid | ||
| fn is_cache_valid(&self, entry: &VersionCacheEntry, frequency_hours: u64) -> bool { | ||
| let now = chrono::Utc::now(); | ||
| let hours_since_check = (now - entry.last_checked).num_hours(); | ||
| let time_valid = hours_since_check < frequency_hours as i64; | ||
|
|
||
| // Also check if the current version matches | ||
| let current_version = env!("CARGO_PKG_VERSION"); | ||
| let version_valid = entry.current_version == current_version; | ||
|
|
||
| time_valid && version_valid | ||
| } |
There was a problem hiding this comment.
Guard against clock skew when validating cache.
If last_checked is in the future, hours_since_check is negative and the entry is incorrectly treated as valid.
fn is_cache_valid(&self, entry: &VersionCacheEntry, frequency_hours: u64) -> bool {
let now = chrono::Utc::now();
- let hours_since_check = (now - entry.last_checked).num_hours();
+ let hours_since_check = (now - entry.last_checked).num_hours();
+ if hours_since_check < 0 {
+ return false;
+ }
let time_valid = hours_since_check < frequency_hours as i64;
// Also check if the current version matches
let current_version = env!("CARGO_PKG_VERSION");
let version_valid = entry.current_version == current_version;
time_valid && version_valid
}📝 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.
| /// Check if a cache entry is still valid | |
| fn is_cache_valid(&self, entry: &VersionCacheEntry, frequency_hours: u64) -> bool { | |
| let now = chrono::Utc::now(); | |
| let hours_since_check = (now - entry.last_checked).num_hours(); | |
| let time_valid = hours_since_check < frequency_hours as i64; | |
| // Also check if the current version matches | |
| let current_version = env!("CARGO_PKG_VERSION"); | |
| let version_valid = entry.current_version == current_version; | |
| time_valid && version_valid | |
| } | |
| /// Check if a cache entry is still valid | |
| fn is_cache_valid(&self, entry: &VersionCacheEntry, frequency_hours: u64) -> bool { | |
| let now = chrono::Utc::now(); | |
| let hours_since_check = (now - entry.last_checked).num_hours(); | |
| if hours_since_check < 0 { | |
| return false; | |
| } | |
| let time_valid = hours_since_check < frequency_hours as i64; | |
| // Also check if the current version matches | |
| let current_version = env!("CARGO_PKG_VERSION"); | |
| let version_valid = entry.current_version == current_version; | |
| time_valid && version_valid | |
| } |
🤖 Prompt for AI Agents
In src/domain/repositories/version_repository.rs around lines 91 to 102, the
cache validation allows entries whose last_checked timestamp is in the future
(clock skew) because num_hours() becomes negative; modify the function to
explicitly guard against future timestamps by returning false if
entry.last_checked > now, otherwise compute hours_since_check normally and
proceed with the existing frequency and version checks. Ensure you compare
chrono::DateTime values directly for the future check and only cast num_hours()
after confirming now >= last_checked.
| // Find git root once at the beginning using the first file | ||
| let git_root = files_to_process | ||
| .first() | ||
| .map(|(first_file, _)| first_file) | ||
| .and_then(|path| LocalGitRepositoryClient::get_repository_root(path)) | ||
| .ok_or_else(|| { | ||
| GitTypeError::ExtractionFailed("Git repository not found".to_string()) | ||
| })?; |
There was a problem hiding this comment.
Handle empty inputs and non-git paths without aborting
This new guard now returns ExtractionFailed whenever files_to_process is empty or when the first path is outside a git repo. Both cases previously worked (we simply returned an empty chunk list or used the absolute path), and several flows still rely on that—for example, CLI calls where nothing matches a filter, or test fixtures in temp dirs that aren’t part of a repo. After this change those scenarios now error out before any extraction runs. Please keep the early-return/parent-path fallback so we don’t regress these code paths. Based on learnings
Here is a possible fix:
+ if files_to_process.is_empty() {
+ progress.set_file_counts(StepType::Extracting, 0, 0, None);
+ return Ok(Vec::new());
+ }
+
- let git_root = files_to_process
- .first()
- .map(|(first_file, _)| first_file)
- .and_then(|path| LocalGitRepositoryClient::get_repository_root(path))
- .ok_or_else(|| {
- GitTypeError::ExtractionFailed("Git repository not found".to_string())
- })?;
+ let (first_path, _) = files_to_process.first().expect("checked above");
+ let git_root = LocalGitRepositoryClient::get_repository_root(first_path)
+ .or_else(|| first_path.parent().map(|p| p.to_path_buf()))
+ .unwrap_or_else(|| first_path.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.
| // Find git root once at the beginning using the first file | |
| let git_root = files_to_process | |
| .first() | |
| .map(|(first_file, _)| first_file) | |
| .and_then(|path| LocalGitRepositoryClient::get_repository_root(path)) | |
| .ok_or_else(|| { | |
| GitTypeError::ExtractionFailed("Git repository not found".to_string()) | |
| })?; | |
| // Find git root once at the beginning using the first file | |
| if files_to_process.is_empty() { | |
| progress.set_file_counts(StepType::Extracting, 0, 0, None); | |
| return Ok(Vec::new()); | |
| } | |
| let (first_path, _) = files_to_process.first().expect("checked above"); | |
| let git_root = LocalGitRepositoryClient::get_repository_root(first_path) | |
| .or_else(|| first_path.parent().map(|p| p.to_path_buf())) | |
| .unwrap_or_else(|| first_path.clone()); |
| /// Compare two version strings and determine if the first is newer | ||
| fn is_version_newer(latest: &str, current: &str) -> bool { | ||
| let latest_parts = Self::parse_version(latest); | ||
| let current_parts = Self::parse_version(current); | ||
|
|
||
| match (latest_parts, current_parts) { | ||
| (Ok(latest), Ok(current)) => { | ||
| for (l, c) in latest.iter().zip(current.iter()) { | ||
| if l > c { | ||
| return true; | ||
| } else if l < c { | ||
| return false; | ||
| } | ||
| } | ||
| latest.len() > current.len() | ||
| } | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix version comparison: treat trailing zeros as equal.
Current logic marks 1.2.0 newer than 1.2, which is incorrect. Trim trailing zeros before length comparison.
- fn is_version_newer(latest: &str, current: &str) -> bool {
- let latest_parts = Self::parse_version(latest);
- let current_parts = Self::parse_version(current);
-
- match (latest_parts, current_parts) {
- (Ok(latest), Ok(current)) => {
- for (l, c) in latest.iter().zip(current.iter()) {
- if l > c {
- return true;
- } else if l < c {
- return false;
- }
- }
- latest.len() > current.len()
- }
- _ => false,
- }
- }
+ fn is_version_newer(latest: &str, current: &str) -> bool {
+ let mut latest = match Self::parse_version(latest) { Ok(v) => v, Err(_) => return false };
+ let mut current = match Self::parse_version(current) { Ok(v) => v, Err(_) => return false };
+ while latest.last().is_some() && latest.last() == Some(&0) { latest.pop(); }
+ while current.last().is_some() && current.last() == Some(&0) { current.pop(); }
+ for (l, c) in latest.iter().zip(current.iter()) {
+ if l > c { return true; }
+ if l < c { return false; }
+ }
+ latest.len() > current.len()
+ }📝 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.
| /// Compare two version strings and determine if the first is newer | |
| fn is_version_newer(latest: &str, current: &str) -> bool { | |
| let latest_parts = Self::parse_version(latest); | |
| let current_parts = Self::parse_version(current); | |
| match (latest_parts, current_parts) { | |
| (Ok(latest), Ok(current)) => { | |
| for (l, c) in latest.iter().zip(current.iter()) { | |
| if l > c { | |
| return true; | |
| } else if l < c { | |
| return false; | |
| } | |
| } | |
| latest.len() > current.len() | |
| } | |
| _ => false, | |
| } | |
| } | |
| /// Compare two version strings and determine if the first is newer | |
| fn is_version_newer(latest: &str, current: &str) -> bool { | |
| let mut latest = match Self::parse_version(latest) { Ok(v) => v, Err(_) => return false }; | |
| let mut current = match Self::parse_version(current) { Ok(v) => v, Err(_) => return false }; | |
| while latest.last().is_some() && latest.last() == Some(&0) { latest.pop(); } | |
| while current.last().is_some() && current.last() == Some(&0) { current.pop(); } | |
| for (l, c) in latest.iter().zip(current.iter()) { | |
| if l > c { return true; } | |
| if l < c { return false; } | |
| } | |
| latest.len() > current.len() | |
| } |
🤖 Prompt for AI Agents
In src/domain/services/version_service.rs around lines 33 to 51, the comparison
treats versions like "1.2.0" as newer than "1.2"; to fix it, trim trailing zeros
from both parsed version component vectors before performing the final
length-based comparison: after parsing, remove any trailing zeros from
latest_parts and current_parts, then iterate over the zipped components to
compare numerically as before, and finally use the trimmed lengths to decide
(longer => newer); ensure non-numeric parse errors still return false.
| match repository_ref { | ||
| spec if spec.contains('@') => Self::parse_ssh_format(spec), | ||
| spec if spec.starts_with("http") => Self::parse_https_format(spec), | ||
| spec if spec.contains('/') && !spec.contains(' ') => Self::parse_short_format(spec), | ||
| _ => Err(GitTypeError::InvalidRepositoryFormat(format!( | ||
| "Unsupported repository format: {}", | ||
| repository_ref | ||
| ))), | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid misclassifying scheme URIs (e.g., ssh://) and guard short refs against ':'
Current dispatch treats any string with '@' as SCP‑style SSH and any with '/' as short, which misroutes inputs like ssh://git@host/owner/name and accepts invalid github.com:owner/name as short. Tighten the conditions and trim input.
Apply:
- pub fn parse(repository_ref: &str) -> Result<GitRepositoryRef> {
- match repository_ref {
- spec if spec.contains('@') => Self::parse_ssh_format(spec),
- spec if spec.starts_with("http") => Self::parse_https_format(spec),
- spec if spec.contains('/') && !spec.contains(' ') => Self::parse_short_format(spec),
+ pub fn parse(repository_ref: &str) -> Result<GitRepositoryRef> {
+ let spec = repository_ref.trim();
+ match spec {
+ s if s.starts_with("http://") || s.starts_with("https://") => {
+ Self::parse_https_format(s)
+ }
+ // SCP-like only: must contain '@' and ':' and not be a scheme URL
+ s if s.contains('@') && s.contains(':') && !s.contains("://") => {
+ Self::parse_ssh_format(s)
+ }
+ // Short refs must not contain ':' or a scheme
+ s if s.contains('/') && !s.contains(' ') && !s.contains(':') && !s.contains("://") => {
+ Self::parse_short_format(s)
+ }
_ => Err(GitTypeError::InvalidRepositoryFormat(format!(
"Unsupported repository format: {}",
- repository_ref
+ spec
))),
}
}📝 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.
| match repository_ref { | |
| spec if spec.contains('@') => Self::parse_ssh_format(spec), | |
| spec if spec.starts_with("http") => Self::parse_https_format(spec), | |
| spec if spec.contains('/') && !spec.contains(' ') => Self::parse_short_format(spec), | |
| _ => Err(GitTypeError::InvalidRepositoryFormat(format!( | |
| "Unsupported repository format: {}", | |
| repository_ref | |
| ))), | |
| } | |
| } | |
| pub fn parse(repository_ref: &str) -> Result<GitRepositoryRef> { | |
| let spec = repository_ref.trim(); | |
| match spec { | |
| s if s.starts_with("http://") || s.starts_with("https://") => { | |
| Self::parse_https_format(s) | |
| } | |
| // SCP-like only: must contain '@' and ':' and not be a scheme URL | |
| s if s.contains('@') && s.contains(':') && !s.contains("://") => { | |
| Self::parse_ssh_format(s) | |
| } | |
| // Short refs must not contain ':' or a scheme | |
| s if s.contains('/') && !s.contains(' ') && !s.contains(':') && !s.contains("://") => { | |
| Self::parse_short_format(s) | |
| } | |
| _ => Err(GitTypeError::InvalidRepositoryFormat(format!( | |
| "Unsupported repository format: {}", spec | |
| ))), | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/infrastructure/git/git_repository_ref_parser.rs around lines 8 to 17, the
dispatcher misclassifies URIs like "ssh://..." as SCP-style and accepts refs
with ':' as short form; trim repository_ref first, then refine the branches:
check explicit URI schemes first (e.g., starts_with("ssh://") or
starts_with("http")) to route to parse_ssh_format or parse_https_format; detect
SCP-style SSH only when the trimmed string contains '@' AND contains ':' AND
does not contain "://"; only treat the short format when it contains '/' and
does NOT contain ':' or "://"; on failure return the same
InvalidRepositoryFormat error. Ensure trimming is applied once at the top.
| fn parse_ssh_format(repo_spec: &str) -> Result<GitRepositoryRef> { | ||
| let (host_part, repo_part) = repo_spec.split_once(':').ok_or_else(|| { | ||
| GitTypeError::InvalidRepositoryFormat("Invalid SSH repository format".to_string()) | ||
| })?; | ||
|
|
||
| let origin = host_part | ||
| .split('@') | ||
| .nth(1) | ||
| .unwrap_or("github.com") | ||
| .to_string(); | ||
|
|
||
| let (owner, name) = repo_part | ||
| .strip_suffix(".git") | ||
| .unwrap_or(repo_part) | ||
| .split_once('/') | ||
| .ok_or_else(|| { | ||
| GitTypeError::InvalidRepositoryFormat("Invalid repository path format".to_string()) | ||
| })?; | ||
|
|
||
| Ok(GitRepositoryRef { | ||
| origin, | ||
| owner: owner.to_string(), | ||
| name: name.to_string(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
SSH parsing: don’t default origin; validate non-empty owner/name; accept optional leading “/” after colon
Falling back to github.com hides malformed inputs. Also guard empty segments and normalize repo_part.
- fn parse_ssh_format(repo_spec: &str) -> Result<GitRepositoryRef> {
+ fn parse_ssh_format(repo_spec: &str) -> Result<GitRepositoryRef> {
let (host_part, repo_part) = repo_spec.split_once(':').ok_or_else(|| {
GitTypeError::InvalidRepositoryFormat("Invalid SSH repository format".to_string())
})?;
- let origin = host_part
- .split('@')
- .nth(1)
- .unwrap_or("github.com")
- .to_string();
+ let origin = host_part
+ .split('@')
+ .nth(1)
+ .ok_or_else(|| {
+ GitTypeError::InvalidRepositoryFormat(
+ "Invalid SSH repository format (missing '@')".to_string(),
+ )
+ })?
+ .to_string();
- let (owner, name) = repo_part
- .strip_suffix(".git")
- .unwrap_or(repo_part)
- .split_once('/')
+ // Support "git@host:/owner/name(.git)" and validate parts
+ let repo_part = repo_part.trim_start_matches('/');
+ let (owner, name_raw) = repo_part
+ .split_once('/')
.ok_or_else(|| {
GitTypeError::InvalidRepositoryFormat("Invalid repository path format".to_string())
})?;
+ let name = name_raw.strip_suffix(".git").unwrap_or(name_raw);
+ if owner.is_empty() || name.is_empty() {
+ return Err(GitTypeError::InvalidRepositoryFormat(
+ "Invalid repository path format".to_string(),
+ ));
+ }
Ok(GitRepositoryRef {
origin,
owner: owner.to_string(),
name: name.to_string(),
})
}🤖 Prompt for AI Agents
In src/infrastructure/git/git_repository_ref_parser.rs around lines 19 to 43,
the SSH parser currently defaults a missing host to "github.com", doesn't accept
an optional leading '/' after the colon, and doesn't validate that owner and
name are non-empty; change it to (1) extract the host from host_part by taking
the segment after '@' (or the whole host_part if no '@') and return a
GitTypeError::InvalidRepositoryFormat when that host is empty instead of
defaulting to github.com; (2) normalize repo_part by trimming a leading '/'
before processing; (3) strip the ".git" suffix then split_once('/') and
explicitly check that both owner and name are non-empty, returning the same
InvalidRepositoryFormat error when either is empty; finally construct
GitRepositoryRef with the validated origin, owner and name.
| #[cfg(feature = "test-mocks")] | ||
| { | ||
| Ok(PathBuf::from("/tmp/test")) | ||
| } |
There was a problem hiding this comment.
Hard‑coded “/tmp/test” breaks on Windows and may not exist.
Use a cross‑platform temp location and ensure it exists.
- Ok(PathBuf::from("/tmp/test"))
+ {
+ let dir = std::env::temp_dir().join("gittype-test");
+ std::fs::create_dir_all(&dir).map_err(|e| {
+ GitTypeError::ExtractionFailed(format!(
+ "Failed to create test app data dir {}: {}",
+ dir.display(),
+ e
+ ))
+ })?;
+ Ok(dir)
+ }📝 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.
| #[cfg(feature = "test-mocks")] | |
| { | |
| Ok(PathBuf::from("/tmp/test")) | |
| } | |
| #[cfg(feature = "test-mocks")] | |
| { | |
| let dir = std::env::temp_dir().join("gittype-test"); | |
| std::fs::create_dir_all(&dir).map_err(|e| { | |
| GitTypeError::ExtractionFailed(format!( | |
| "Failed to create test app data dir {}: {}", | |
| dir.display(), | |
| e | |
| )) | |
| })?; | |
| Ok(dir) | |
| } |
🤖 Prompt for AI Agents
In src/infrastructure/storage/app_data_provider.rs around lines 27 to 30, the
code returns a hard-coded "/tmp/test" which fails on Windows and may not exist;
replace it with std::env::temp_dir(), append a suitable subdirectory (e.g.
"test" or crate-specific folder), create the directory if missing using
std::fs::create_dir_all, and return the resulting PathBuf wrapped in Ok,
propagating any filesystem errors as appropriate.
| std::fs::read_dir(dir_path) | ||
| .unwrap_or_else(|_| std::fs::read_dir(".").unwrap()) | ||
| .filter_map(|entry| entry.ok()) | ||
| .map(|entry| entry.path()) | ||
| .filter(|path| path.is_file()) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
Fix the fallback that wipes unrelated files on read errors
If read_dir(dir_path) errors (permission, transient FS issue, etc.) we fall back to read_dir("."). clear_cache() and every other caller then happily delete whatever is in the process working directory. One permission glitch on the cache dir can nuke unrelated project files. Please stop falling back to . and surface the error or return an empty list instead.
- std::fs::read_dir(dir_path)
- .unwrap_or_else(|_| std::fs::read_dir(".").unwrap())
- .filter_map(|entry| entry.ok())
- .map(|entry| entry.path())
- .filter(|path| path.is_file())
- .collect()
+ match std::fs::read_dir(dir_path) {
+ Ok(entries) => entries
+ .filter_map(|entry| entry.ok())
+ .map(|entry| entry.path())
+ .filter(|path| path.is_file())
+ .collect(),
+ Err(_) => Vec::new(),
+ }📝 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.
| std::fs::read_dir(dir_path) | |
| .unwrap_or_else(|_| std::fs::read_dir(".").unwrap()) | |
| .filter_map(|entry| entry.ok()) | |
| .map(|entry| entry.path()) | |
| .filter(|path| path.is_file()) | |
| .collect() | |
| } | |
| match std::fs::read_dir(dir_path) { | |
| Ok(entries) => entries | |
| .filter_map(|entry| entry.ok()) | |
| .map(|entry| entry.path()) | |
| .filter(|path| path.is_file()) | |
| .collect(), | |
| Err(_) => Vec::new(), | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/infrastructure/storage/compressed_file_storage.rs around lines 95 to 101,
the code falls back to reading the process CWD on read_dir(dir_path) errors
which can cause deletion of unrelated files; instead, stop using read_dir(".")
and either propagate the error to the caller or return an empty Vec<PathBuf>.
Modify this function to handle the read_dir(dir_path) Result explicitly: on Err,
either map the error to a returned Err (adjust the function signature to return
Result<Vec<PathBuf>, std::io::Error>) or log and return Ok(Vec::new()) so
callers won't act on the wrong directory; remove the current unwrap_or_else
fallback to ".".
| if let Ok(chunks) = extract_from_file_for_test(file_path, language.name()) { | ||
| all_chunks.extend(chunks); |
There was a problem hiding this comment.
Don't swallow extraction failures
By wrapping extract_from_file_for_test in if let Ok(...), any parsing/extraction error quietly disappears and the tests still pass, masking real regressions. These helpers should surface failures just like the production extractor so the test suite goes red when chunk extraction breaks. Please propagate the error instead of discarding it.
- if let Ok(chunks) = extract_from_file_for_test(file_path, language.name()) {
- all_chunks.extend(chunks);
- }
+ let chunks = extract_from_file_for_test(file_path, language.name())?;
+ all_chunks.extend(chunks);📝 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.
| if let Ok(chunks) = extract_from_file_for_test(file_path, language.name()) { | |
| all_chunks.extend(chunks); | |
| let chunks = extract_from_file_for_test(file_path, language.name())?; | |
| all_chunks.extend(chunks); |
🤖 Prompt for AI Agents
In tests/integration/mod.rs around lines 71-72, the test currently swallows
extraction errors by using `if let Ok(...)` which hides failures; change the
call to propagate errors so the test fails on extraction problems — replace the
`if let Ok(chunks) = extract_from_file_for_test(...) {
all_chunks.extend(chunks); }` pattern with a form that returns or bubbles up the
error (use the `?` operator if the test fn returns Result, or call
`.expect("chunk extraction failed")`/`.unwrap()` if it must panic), ensuring
extraction errors are not silently ignored.
| if let Ok(chunks) = extract_from_file_for_test(&file_path, language.name()) { | ||
| all_chunks.extend(chunks); |
There was a problem hiding this comment.
Propagate extraction errors in test helper
Same issue here: swallowing the Err means broken extraction won’t fail the test run. Let the Result bubble up so tests remain trustworthy.
- if let Ok(chunks) = extract_from_file_for_test(&file_path, language.name()) {
- all_chunks.extend(chunks);
- }
+ let chunks = extract_from_file_for_test(&file_path, language.name())?;
+ all_chunks.extend(chunks);🤖 Prompt for AI Agents
In tests/integration/mod.rs around lines 91-92, the helper currently swallows
extraction errors with an if let Ok(...) pattern, which hides failures; replace
the if-let with a direct call that propagates the Result (use the ? operator) so
any Err bubbles up to the test runner, and update the helper function's
signature to return Result<..., E> (or anyhow::Result) to accommodate
propagation; ensure callers handle the propagated Result accordingly.
| fn create_test_cache() -> ChallengeRepository { | ||
| ChallengeRepository::with_cache_dir(PathBuf::from("/mock/cache")) | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix cache dir setup: current path breaks the tests.
create_test_cache hard-codes /mock/cache. On a typical CI/Local run the process cannot create folders under /, so save_challenges returns Err and every assertion expecting Ok(()) will fail. Even if the directory existed, all tests would contend for the same path; when the suite runs in parallel, clear_cache/invalidate_repository from one test wipe the data another test is asserting on. Please give each test its own writable temp directory (e.g., tempfile::TempDir) and keep it alive for the test’s duration before calling ChallengeRepository::with_cache_dir. For example:
- fn create_test_cache() -> ChallengeRepository {
- ChallengeRepository::with_cache_dir(PathBuf::from("/mock/cache"))
- }
+ fn create_test_cache() -> (ChallengeRepository, tempfile::TempDir) {
+ let dir = tempfile::TempDir::new().expect("temp cache dir");
+ let cache = ChallengeRepository::with_cache_dir(dir.path().to_path_buf());
+ (cache, dir)
+ }Then destructure (cache, _dir) in each test so the temp directory isn’t dropped prematurely. This keeps the tests hermetic and fixes the permission failures.
📝 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.
| fn create_test_cache() -> ChallengeRepository { | |
| ChallengeRepository::with_cache_dir(PathBuf::from("/mock/cache")) | |
| } | |
| fn create_test_cache() -> (ChallengeRepository, tempfile::TempDir) { | |
| let dir = tempfile::TempDir::new().expect("temp cache dir"); | |
| let cache = ChallengeRepository::with_cache_dir(dir.path().to_path_buf()); | |
| (cache, dir) | |
| } |
🤖 Prompt for AI Agents
In tests/unit/domain/repositories/challenge_repository_tests.rs around lines 34
to 37, create_test_cache currently returns a ChallengeRepository using a
hard-coded "/mock/cache" which fails on CI and causes test interference; change
it to create a unique writable temp directory per test using tempfile::TempDir,
keep the TempDir alive for the test's duration (return or destructure as (repo,
_dir) in each test so the TempDir isn't dropped), and pass TempDir.path()
(converted to PathBuf) into ChallengeRepository::with_cache_dir so each test
gets its own hermetic, writable cache directory.
unhappychoice
deleted the
feature/clean-architecture-infrastructure-refactoring
branch
1 participant
Summary
Changes
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Changes
Refactor
Tests
Chores