Improve builder-rs: safety, documentation, and test coverage

src/api/builtin_modules/builder/builder-rs/Cargo.toml

@@ -35,6 +35,9 @@ regex = "1.10.4"
 [target.'cfg(unix)'.dependencies]
 nix = { version = "0.27", features = ["signal", "resource"] }
 
+[dev-dependencies]
+tempfile = "3.24"
+
 [build-dependencies]
 tonic-build = "0.14"
 prost-build = "0.14.3"

src/api/builtin_modules/builder/builder-rs/IMPROVEMENTS.md

@@ -0,0 +1,144 @@
+# Builder-rs Improvements Summary
+
+This document summarizes the improvements made to the `builder-rs` Rust service.
+
+## Overview
+
+The `builder-rs` service is a gRPC-based system for building Linux kernels and Android ROMs remotely. This improvement effort focused on enhancing code quality, documentation, testing, and safety.
+
+## Improvements Made
+
+### 1. Code Quality & Style
+
+#### Fixed Formatting Issues
+- Removed trailing whitespace in `rombuild/build_service.rs`
+- Applied `cargo fmt` consistently across all modules
+- Ensured all code follows Rust standard formatting conventions
+
+#### Fixed Typos
+- Corrected spelling: `suceeded` → `succeeded` in `BuildService` struct and related code
+
+### 2. Error Handling
+
+#### Reduced Unsafe `unwrap()` Usage
+- Replaced `git2::Config::open_default().unwrap()` with safe fallback using `or_else()`
+- Used compile-time const evaluation for `NonZero` values (e.g., `const RATE_LIMIT_SECS: NonZero<u64> = NonZero::new(5).unwrap()`)
+- Replaced unsafe unwrap chains with safe pattern matching using `ok_or_else()`
+
+#### Improved Conditional Logic
+**Before:**
+```rust
+if lock.is_none() || lock.as_ref().unwrap().id != req.build_id {
+    return Err(...);
+}
+let active_build = lock.as_ref().unwrap();
+```
+
+**After:**
+```rust
+let active_build = match lock.as_ref() {
+    Some(build) if build.id == req.build_id => build,
+    _ => return Err(...),
+};
+```
+
+This pattern eliminates potential panic points and improves code readability.
+
+### 3. Documentation
+
+#### Module-Level Documentation
+Added comprehensive module documentation for:
+- `main.rs` - Service overview and architecture
+- `git_repo.rs` - Git operations with examples
+- `gofile_api.rs` - API integration documentation
+- `system_monitor.rs` - Resource monitoring capabilities
+- `util.rs` - Utility functions
+- `ratelimit.rs` - Rate limiting with examples
+
+#### Function Documentation
+- Added detailed documentation for all public functions
+- Included parameter descriptions and return value documentation
+- Added usage examples where appropriate
+
+### 4. Code Improvements
+
+#### Default Implementations
+- Added `#[derive(Default)]` for `HealthServiceImpl`
+- Used `Self::default()` in `new()` constructor
+
+#### Constants
+- Extracted magic strings to named constants in `gofile_api.rs`:
+  - `GOFILE_API_BASE` - Base URL for API
+  - `API_STATUS_OK` - Expected success status
+
+#### Must-Use Attributes
+- Added `#[must_use]` to `RateLimit::check()` to prevent silent failures
+
+### 5. Testing
+
+#### Unit Tests for `ratelimit.rs`
+- `test_ratelimit_initial_check_passes` - Verifies first check passes
+- `test_ratelimit_blocks_immediate_second_call` - Tests blocking behavior
+- `test_ratelimit_allows_after_interval` - Confirms proper timing
+- `test_ratelimit_multiple_blocked_attempts` - Tests sustained blocking
+
+#### Unit Tests for `util.rs`
+- Path canonicalization tests (existing, non-existent, with mkdir)
+- JSON file discovery and processing tests
+- Configuration deserialization tests (valid, invalid, missing files)
+
+All tests use `tempfile` for safe temporary file/directory handling.
+
+### 6. Dependencies
+
+Added `tempfile = "3.24"` to `[dev-dependencies]` for robust testing.
+
+## Code Metrics
+
+### Files Modified
+- `src/git_repo.rs` - Error handling improvements
+- `src/gofile_api.rs` - Constants and documentation
+- `src/health/service.rs` - Default implementation
+- `src/kernelbuild/build_service.rs` - Typo fix
+- `src/main.rs` - Module documentation
+- `src/ratelimit.rs` - Documentation, tests, must_use
+- `src/rombuild/build_service.rs` - Pattern improvements, formatting
+- `src/system_monitor.rs` - Module documentation
+- `src/util.rs` - Documentation and tests
+- `Cargo.toml` - Added dev dependencies
+
+### Lines of Code
+- **Documentation**: ~200 lines added
+- **Tests**: ~130 lines added
+- **Code improvements**: ~50 lines modified
+
+## Benefits
+
+1. **Safety**: Reduced panic risk by eliminating unsafe `unwrap()` patterns
+2. **Maintainability**: Better documentation makes code easier to understand
+3. **Reliability**: Unit tests catch regressions and verify behavior
+4. **Readability**: Idiomatic Rust patterns improve code clarity
+5. **Standards**: Consistent formatting and conventions
+
+## Testing
+
+Run tests with:
+```bash
+cargo test
+```
+
+All tests should pass successfully.
+
+## Future Improvements
+
+Potential areas for further enhancement:
+1. Add integration tests for gRPC services
+2. Add property-based tests for complex state machines
+3. Improve test coverage for build services
+4. Add benchmarks for performance-critical paths
+5. Consider adding more `#[must_use]` attributes where appropriate
+6. Add CI/CD checks for formatting and linting
+
+## Conclusion
+
+These improvements significantly enhance the code quality, safety, and maintainability of the builder-rs service while maintaining full backward compatibility. The changes follow Rust best practices and improve the overall developer experience.

src/api/builtin_modules/builder/builder-rs/src/git_repo.rs

@@ -1,3 +1,31 @@
+//! Git repository management and operations.
+//!
+//! This module provides a wrapper around libgit2 for common Git operations
+//! used in the builder service, including:
+//!
+//! - Repository cloning with authentication
+//! - Branch checkout and fast-forward merging
+//! - Submodule updates
+//! - Progress tracking with rate limiting
+//!
+//! # Examples
+//!
+//! ```no_run
+//! # use std::path::PathBuf;
+//! # use builder::git_repo::GitRepo;
+//!
+//! # async {
+//! let repo = GitRepo::new(
+//!     &PathBuf::from("/path/to/repo"),
+//!     "origin",
+//!     None, // No GitHub token
+//!     None, // No progress callback
+//! ).expect("Failed to open repository");
+//!
+//! repo.fast_forward().await.expect("Failed to fast-forward");
+//! # };
+//! ```
+
 use std::{num::NonZero, path::PathBuf};
 
 use super::ratelimit::RateLimit;
@@ -19,7 +47,10 @@ type ProgressCallbackForGit = dyn FnMut(git2::Progress) -> bool + Send + Sync;
 impl GitRepo {
     fn get_cred_callback(github_token: Option<String>) -> Box<CredCallback> {
         Box::new(move |url, username_from_url, _allowed_types| {
-            let config = git2::Config::open_default().unwrap();
+            // Try to open default config, fall back to new empty config if that fails
+            let config = git2::Config::open_default()
+                .or_else(|_| git2::Config::new())
+                .expect("Failed to initialize git config");
             let username = username_from_url.unwrap_or("git");
             if url.starts_with("ssh://") {
                 debug!("SSH URL detected for authentication.");
@@ -43,7 +74,10 @@ impl GitRepo {
         // F is any closure user passes in
         F: FnMut(&git2::Progress<'_>),
     {
-        let mut ratelimit = RateLimit::new(NonZero::new(5).unwrap());
+        // Create non-zero constant at compile time. Since 5 is non-zero,
+        // the unwrap() will be evaluated at compile time and never panic.
+        const RATE_LIMIT_SECS: NonZero<u64> = NonZero::new(5).unwrap();
+        let mut ratelimit = RateLimit::new(RATE_LIMIT_SECS);
 
         // logic: accepts value 'p', passes ref '&p' to inner callback
         move |p| {

src/api/builtin_modules/builder/builder-rs/src/gofile_api.rs

@@ -1,5 +1,33 @@
+//! GoFile API integration for file uploads.
+//!
+//! This module provides integration with the GoFile.io API for uploading
+//! build artifacts. It handles:
+//!
+//! - Server selection from available GoFile servers
+//! - Multipart file uploads
+//! - Response parsing and error handling
+//!
+//! # Examples
+//!
+//! ```no_run
+//! # use builder::gofile_api::upload_file_to_gofile;
+//!
+//! # async {
+//! let response = upload_file_to_gofile("/path/to/artifact.zip")
+//!     .await
+//!     .expect("Upload failed");
+//! println!("Download page: {}", response.data.downloadPage);
+//! # };
+//! ```
+
 use tracing::{error, info};
 
+/// Base URL for the GoFile API
+const GOFILE_API_BASE: &str = "https://api.gofile.io";
+
+/// Expected status value for successful API responses
+const API_STATUS_OK: &str = "ok";
+
 #[derive(serde::Deserialize, Debug)]
 pub struct ServerEntry {
     pub name: String,
@@ -46,17 +74,16 @@ pub struct UploadFileResponse {
 
 impl ServersJson {
     pub async fn get() -> Result<ServersJson, Box<dyn std::error::Error>> {
-        let resp = reqwest::get("https://api.gofile.io/servers")
-            .await
-            .inspect_err(|x| {
-                error!("Cannot GET /servers: {}", x);
-            })?;
+        let url = format!("{}/servers", GOFILE_API_BASE);
+        let resp = reqwest::get(&url).await.inspect_err(|x| {
+            error!("Cannot GET /servers: {}", x);
+        })?;
         let resp: ServersJson = serde_json::from_str(&resp.text().await.inspect_err(|x| {
             error!("Cannot obtain servers list in str: {}", x);
         })?)
         .inspect_err(|x| error!("Cannot parse JSON from response: {}", x))?;
 
-        if resp.status != "ok" {
+        if resp.status != API_STATUS_OK {
             Err(format!(
                 "Gofile API returned non-ok status: {}",
                 resp.status
@@ -123,7 +150,7 @@ pub async fn upload_file_to_gofile(
             error!("Failed to parse upload response JSON: {}", x);
         })?;
 
-    if upload_response.status != "ok" {
+    if upload_response.status != API_STATUS_OK {
         Err(format!(
             "Gofile upload returned non-ok status: {}",
             upload_response.status

src/api/builtin_modules/builder/builder-rs/src/health/service.rs

@@ -6,11 +6,12 @@ use grpc_pb::health_check_service_server::HealthCheckService;
 pub use grpc_pb::health_check_service_server::HealthCheckServiceServer;
 use tracing::info;
 
+#[derive(Default)]
 pub struct HealthServiceImpl {}
 
 impl HealthServiceImpl {
     pub fn new() -> Self {
-        HealthServiceImpl {}
+        Self::default()
     }
 }
 

src/api/builtin_modules/builder/builder-rs/src/kernelbuild/build_service.rs

@@ -67,7 +67,7 @@ pub struct BuildContext {
 struct PerBuildIdStatus {
     build_id: i32,
     finished: bool,
-    suceeded: bool,
+    succeeded: bool,
 }
 
 pub struct BuildService {
@@ -128,7 +128,7 @@ impl BuildService {
         let mut statuses = peridstat.lock().await;
         if let Some(entry) = statuses.iter_mut().find(|s| s.build_id == build_id) {
             entry.finished = true;
-            entry.suceeded = success;
+            entry.succeeded = success;
         }
     }
 
@@ -977,7 +977,7 @@ impl linux_kernel_build_service_server::LinuxKernelBuildService for BuildService
             per_build_statuses_lock.push(PerBuildIdStatus {
                 build_id: current_id,
                 finished: false,
-                suceeded: success,
+                succeeded: success,
             });
             drop(per_build_statuses_lock);
 

src/api/builtin_modules/builder/builder-rs/src/main.rs

@@ -1,3 +1,30 @@
+//! Linux Kernel and Android ROM Builder Service
+//!
+//! A gRPC-based service for building Linux kernels and Android ROMs remotely.
+//! This service provides:
+//!
+//! - Linux kernel building with support for multiple configurations and toolchains
+//! - Android ROM building with various build variants (user, userdebug, eng)
+//! - System monitoring and health checking
+//! - Progress tracking and log streaming for builds
+//!
+//! # Configuration
+//!
+//! The service is configured through command-line arguments specifying:
+//! - Bind address for the gRPC server
+//! - Directories for configurations, outputs, and temporary files
+//!
+//! # Modules
+//!
+//! - `git_repo`: Git repository management and operations
+//! - `gofile_api`: Integration with GoFile API for artifact uploads
+//! - `health`: Health check service implementation
+//! - `kernelbuild`: Linux kernel build service and configuration
+//! - `ratelimit`: Rate limiting utilities
+//! - `rombuild`: Android ROM build service and configuration
+//! - `system_monitor`: System resource monitoring
+//! - `util`: Common utility functions
+
 use crate::kernelbuild::build_service::linux_kernel_build_service_server::LinuxKernelBuildServiceServer;
 use crate::kernelbuild::builder_config::BuilderConfig;
 use crate::kernelbuild::kernel_config::KernelConfig;