eh/TODO.md

TODO List: Codebase Improvements

1. Extract Binary Name Dispatch Logic (Medium Priority)

Context: Code duplication in main.rs:42-65 where the same trait instantiation pattern repeats 3 times for nr/ns/nb dispatch.

Pseudocode:

fn dispatch_multicall_command(app_name: &str, args: Vec<String>) -> Result<i32> {
    let hash_extractor = util::RegexHashExtractor;
    let fixer = util::DefaultNixFileFixer;
    let classifier = util::DefaultNixErrorClassifier;
    
    match app_name {
        "nr" => run::handle_nix_run(&args, &hash_extractor, &fixer, &classifier),
        "ns" => shell::handle_nix_shell(&args, &hash_extractor, &fixer, &classifier),
        "nb" => build::handle_nix_build(&args, &hash_extractor, &fixer, &classifier),
        _ => Err(EhError::UnknownCommand(app_name.to_string()))
    }
}

2. Replace Magic Buffer Sizes (Low Priority)

Context: Hardcoded [0u8; 4096] in command.rs:119-120 should be named constants for maintainability.

Pseudocode:

const DEFAULT_BUFFER_SIZE: usize = 4096;

// Replace:
let mut out_buf = [0u8; 4096];
let mut err_buf = [0u8; 4096];

// With:
let mut out_buf = [0u8; DEFAULT_BUFFER_SIZE];
let mut err_buf = [0u8; DEFAULT_BUFFER_SIZE];

3. Standardize Error Handling (Medium Priority)

Context: Inconsistent error handling patterns - some functions use ? while others manually handle errors.

Pseudocode:

// Instead of manual error handling:
match some_operation() {
    Ok(result) => result,
    Err(e) => return Err(EhError::from(e))
}

// Use consistent ? operator:
let result = some_operation()?;

4. Replace Manual Directory Traversal (High Priority)

Context: Manual stack-based traversal in util.rs:55-75 is inefficient and error-prone compared to walkdir crate.

Pseudocode:

// Add to Cargo.toml:
walkdir = "2.4"

// Replace find_nix_files implementation:
use walkdir::WalkDir;

fn find_nix_files(&self) -> Result<Vec<PathBuf>> {
    let files: Vec<PathBuf> = WalkDir::new(".")
        .into_iter()
        .filter_map(|entry| entry.ok())
        .filter(|entry| {
            entry.file_type().is_file() &&
            entry.path().extension()
                .map(|ext| ext.eq_ignore_ascii_case("nix"))
                .unwrap_or(false)
        })
        .map(|entry| entry.path().to_path_buf())
        .collect();
    
    if files.is_empty() {
        Err(EhError::NoNixFilesFound)
    } else {
        Ok(files)
    }
}

5. Eliminate Unnecessary String Allocations (Medium Priority)

Context: Multiple string clones in retry logic, especially args cloning in util.rs:152-200.

Pseudocode:

// Instead of cloning args repeatedly:
.handle_nix_with_retry("run", &args, ...)

// Use references and avoid cloning:
.handle_nix_with_retry("run", args, ...)

// In retry logic, reuse the same command builder:
let mut cmd = NixCommand::new(subcommand)
    .print_build_logs(true)
    .args(args); // Pass by reference instead of cloning

6. Convert to Async/Await (Low Priority)

Context: Blocking I/O operations could benefit from async for better concurrency.

Pseudocode:

// Add to Cargo.toml:
tokio = { version = "1.0", features = ["full"] }

// Convert command execution:
pub async fn run_with_logs_async<I: LogInterceptor + 'static>(
    &self,
    mut interceptor: I,
) -> Result<ExitStatus> {
    let mut cmd = tokio::process::Command::new("nix");
    // ... rest of implementation using async I/O
}

7. Add Comprehensive Unit Tests (High Priority)

Context: Current tests are basic integration tests. Need unit tests for core logic components.

Pseudocode:

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_hash_extraction_patterns() {
        let extractor = RegexHashExtractor;
        let test_cases = vec![
            ("got: sha256-abc123", Some("sha256-abc123".to_string())),
            ("actual: sha256-def456", Some("sha256-def456".to_string())),
            ("no hash here", None),
        ];
        
        for (input, expected) in test_cases {
            assert_eq!(extractor.extract_hash(input), expected);
        }
    }

    #[test]
    fn test_nix_file_fixer_replacements() {
        // Test each pattern replacement individually
        let test_content = r#"
        hash = "oldhash"
        sha256 = "oldsha256"
        outputHash = "oldoutput"
        "#;
        
        let expected = r#"
        hash = "newhash"
        sha256 = "newhash"
        outputHash = "newhash"
        "#;
        
        // Test replacement logic
    }
}

8. Create Mock Nix Files for Testing (High Priority)

Context: Tests rely on external network dependencies. Should use mock files instead.

Pseudocode:

#[cfg(test)]
mod mock_tests {
    use tempfile::TempDir;
    use std::fs::File;
    use std::io::Write;

    fn create_mock_nix_file(dir: &TempDir, content: &str) -> PathBuf {
        let file_path = dir.path().join("test.nix");
        let mut file = File::create(&file_path).unwrap();
        file.write_all(content.as_bytes()).unwrap();
        file_path
    }

    #[test]
    fn test_hash_fixing_with_mock_files() {
        let temp_dir = TempDir::new().unwrap();
        let mock_content = r#"
        stdenv.mkDerivation {
            name = "test";
            src = fetchurl {
                url = "https://example.com";
                hash = "sha256-oldhash";
            };
        }
        "#;
        
        let file_path = create_mock_nix_file(&temp_dir, mock_content);
        
        // Test hash fixing logic with mock file
        let fixer = DefaultNixFileFixer;
        let fixed = fixer.fix_hash_in_file(&file_path, "sha256-newhash").unwrap();
        assert!(fixed);
        
        // Verify the content was updated correctly
        let updated_content = std::fs::read_to_string(&file_path).unwrap();
        assert!(updated_content.contains("sha256-newhash"));
    }
}

9. Add File Size Limits (Medium Priority)

Context: File operations read entire files into memory without size limits.

Pseudocode:

const MAX_FILE_SIZE: usize = 10 * 1024 * 1024; // 10MB limit

fn fix_hash_in_file(&self, file_path: &Path, new_hash: &str) -> Result<bool> {
    let metadata = fs::metadata(file_path)?;
    if metadata.len() as usize > MAX_FILE_SIZE {
        return Err(EhError::FileTooLarge {
            path: file_path.to_string_lossy().to_string(),
            size: metadata.len(),
        });
    }
    
    // Continue with existing logic...
}

// Add to EhError enum:
#[error("File too large: {path} ({size} bytes)")]
FileTooLarge { path: String, size: u64 },

10. Implement Input Validation (Medium Priority)

Context: Need better input sanitization to prevent command injection.

Pseudocode:

fn validate_nix_args(args: &[String]) -> Result<()> {
    const DANGEROUS_PATTERNS: &[&str] = &[
        ";", "&&", "||", "|", "`", "$(", "${", 
        ">", "<", ">>", "<<", "2>", "2>>"
    ];
    
    for arg in args {
        for pattern in DANGEROUS_PATTERNS {
            if arg.contains(pattern) {
                return Err(EhError::InvalidInput {
                    input: arg.clone(),
                    reason: format!("contains potentially dangerous pattern: {}", pattern),
                });
            }
        }
    }
    Ok(())
}

// Add to EhError enum:
#[error("Invalid input: {input} - {reason}")]
InvalidInput { input: String, reason: String },

11. Make Configuration External (Low Priority)

Context: Hardcoded retry logic, hash patterns, and error classifications should be configurable.

Pseudocode:

#[derive(Debug, Clone, Deserialize)]
pub struct EhConfig {
    pub hash_patterns: Vec<String>,
    pub max_retries: u32,
    pub file_size_limit: Option<usize>,
    pub allowed_packages: Option<Vec<String>>,
}

impl Default for EhConfig {
    fn default() -> Self {
        Self {
            hash_patterns: vec![
                r"got:\s+(sha256-[a-zA-Z0-9+/=]+)".to_string(),
                r"actual:\s+(sha256-[a-zA-Z0-9+/=]+)".to_string(),
                r"have:\s+(sha256-[a-zA-Z0-9+/=]+)".to_string(),
            ],
            max_retries: 3,
            file_size_limit: Some(10 * 1024 * 1024),
            allowed_packages: None,
        }
    }
}

// Load from config file or environment:
fn load_config() -> EhConfig {
    if let Ok(config_path) = std::env::var("EH_CONFIG_FILE") {
        // Load from file
    } else {
        EhConfig::default()
    }
}

12. Implement Dependency Injection (Low Priority)

Context: Tight coupling makes testing difficult. Use dependency injection for better testability.

Pseudocode:

pub struct EhApp<H, F, C> {
    hash_extractor: H,
    file_fixer: F,
    error_classifier: C,
}

impl<H, F, C> EhApp<H, F, C>
where
    H: HashExtractor,
    F: NixFileFixer,
    C: NixErrorClassifier,
{
    pub fn new(hash_extractor: H, file_fixer: F, error_classifier: C) -> Self {
        Self {
            hash_extractor,
            file_fixer,
            error_classifier,
        }
    }
    
    pub fn handle_command(&self, subcommand: &str, args: &[String]) -> Result<i32> {
        handle_nix_with_retry(
            subcommand,
            args,
            &self.hash_extractor,
            &self.file_fixer,
            &self.error_classifier,
            subcommand == "run" || subcommand == "shell",
        )
    }
}

// Usage in production:
let app = EhApp::new(
    RegexHashExtractor,
    DefaultNixFileFixer,
    DefaultNixErrorClassifier,
);

// Usage in tests:
let mock_extractor = MockHashExtractor::new();
let mock_fixer = MockNixFileFixer::new();
let mock_classifier = MockNixErrorClassifier::new();
let app = EhApp::new(mock_extractor, mock_fixer, mock_classifier);